Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thank you for cleaning this up. I was not aware that the llvm conversion does not have Op specific patterns, yet.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h | ||
---|---|---|
576 | I wonder whether there is some C++ magic so that can be reused more directly from OpConversionPattern? | |
mlir/lib/Conversion/GPUCommon/ConvertLaunchFuncToRuntimeCalls.cpp | ||
326 | Is op really needed? | |
mlir/lib/Conversion/SPIRVToLLVM/ConvertLaunchFuncToLLVMCalls.cpp | ||
156 | Here, too, I think the uses can also be replaced. | |
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
2296–2297 | Could this be callOp.operands()? | |
2314 | Can this be newOp.getResult(0)? | |
2677 | castOp.getAttrDictionary() | |
2745 | reshapeOp.getAttrDictionary() | |
3254 | op.operands()? | |
3270 | op.operands().getTypes()? |
Thanks Stephan for the review. These comments are all good points. This change is primarily about changing the interface. I would prefer to do the clean up in follow-up changes, which can be done in smaller chunks.
The concrete ops do not expose the same interface as Operation* (e.g. getAttrDictionary).
We could either add those, or add an 'Operation* operator->(). I think the latter would make sense because concrete ops already implicitly convert to Operation*`.
I will make that suggestion and see how it goes.
Very nice, I should have done this longtime ago, thanks!
If you have time, maybe we can also consider passing in a SourceOp::Adaptor instead of raw operands?
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h | ||
---|---|---|
576 | We can refactor (likely just rename) ConvertToLLVMPattern into ConvertToLLVMPatternBase, and have ConvertOpToLLVMPattern be public OpConversionPattern, ConvertToLLVMPattern and ConvertToLLVMPattern be public ConversionPattern, ConvertToLLVMPattern. The current base class is essentially a wrapper around TypeConverter + a couple of utility functions. Actually, ConversionPattern now also has a TypeConverter instance, so we could get rid of the instance we have in ConvertToLLVMPattern and just cast instead. Not in this patch though. | |
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
3823–3824 | I suppose this change is just because we never fail to match, but I ticked on it because everything else uses matchAndRewrite. |
This wouldn't fit OpConversionPattern. I will leave it as is in this change.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
3823–3824 | I forgot to undo this. Done now. |
I wonder whether there is some C++ magic so that can be reused more directly from OpConversionPattern?