[CIR] Add inline interface to CIR dialect #1164
Conversation
This allows the inliner to work with the CIR dialect.
|
A motivating use case: Xilinx/mlir-aie@ |
| @@ -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>(); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
mem2reginclang/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-optof sorts, have you thought about that already?
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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.
This allows the inliner to work with the CIR dialect.