Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR] Add inline interface to CIR dialect #1164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[CIR] Add inline interface to CIR dialect #1164

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion clang/lib/CIR/Dialect/IR/CIRDialect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "mlir/Interfaces/InferTypeOpInterface.h"
#include "mlir/Support/LLVM.h"
#include "mlir/Support/LogicalResult.h"
#include "mlir/Transforms/InliningUtils.h"

using namespace mlir;

Expand Down Expand Up @@ -114,6 +115,40 @@ struct CIROpAsmDialectInterface : public OpAsmDialectInterface {
return AliasResult::NoAlias;
}
};

// Minimal interface to inline region with only one block for now (not handling
// the terminator remapping), assuming everything is inlinable.
struct CIRInlinerInterface : DialectInlinerInterface {
using DialectInlinerInterface::DialectInlinerInterface;
// Always allows inlining.
bool isLegalToInline(Operation *call, Operation *callable,
bool wouldBeCloned) const final override {
return true;
}

// Always allows inlining.
bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
IRMapping &valueMapping) const final override {
return true;
}

// Always allows inlining.
bool isLegalToInline(Operation *op, Region *, bool wouldBeCloned,
IRMapping &) const final override {
return true;
}

// Handle the terminator in the case of a single block
void handleTerminator(Operation *op,
ValueRange valuesToReplace) const final override {
// Only handle cir.return for now
if (auto returnOp = dyn_cast<cir::ReturnOp>(op))
for (auto &&[value, operand] :
llvm::zip(valuesToReplace, returnOp.getOperands()))
value.replaceAllUsesWith(operand);
}
};

} // namespace

/// Dialect initialization, the instance will be owned by the context. This is
Expand All @@ -125,7 +160,7 @@ void cir::CIRDialect::initialize() {
#define GET_OP_LIST
#include "clang/CIR/Dialect/IR/CIROps.cpp.inc"
>();
addInterfaces<CIROpAsmDialectInterface>();
addInterfaces<CIRInlinerInterface, CIROpAsmDialectInterface>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental patches are way to go, I'm just a bit confused here: is this supposed to change any behavior at this point or is this just a skeleton that has no observed behavior? If this is changing behavior I'd expect a testcase and/or implementations for isLegalToInline to be returning false (for now).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not change the behavior of CIR in this project since it is not used but allows other CIR users to use the inliner.
I was worried someone would ask for a test. 😉 An easy one would have been to just use the inliner pass (instead of the function) in opt except that a lot of other stuff is not yet implemented in CIR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking two types of tests:

  • A test similar to clang/test/CIR/Transforms/mem2reg.cir.
  • Better yet if we add a flag to cc1 as well, like we do for mem2reg in clang/test/CIR/Transforms/mem2reg.c
  • Thoughts for the future: looks like we need a flag that enable all MLIR passes altogether, something like a -fclangir-mlir-opt of sorts, have you thought about that already?

Copy link
Collaborator Author

@keryell keryell Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, having a test with --inline in the same way as you suggest with --mem2reg would be nice but even with #1203 there are still some crashes with --inline because it requires an analysis to work to compute the benefit of doing the inlining.
I need to investigate this.
In my project I do not have the problem because I call the low-level API which does not compute any cost model and just do the job. 😄

Copy link
Member

@bcardosolopes bcardosolopes Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, it's fine if you initially add one small test that passes while other wild attempts to use might crash/ fail, as long as we don't enable this by default we should be good to incrementally improve it! Basically hook up the driver flags and other setup and inline an empty function, for example.

}

Operation *cir::CIRDialect::materializeConstant(mlir::OpBuilder &builder,
Expand Down