This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Change ConvertOpToLLVMPattern::matchAndRewrite argument to concrete operand type.
ClosedPublic

Authored by csigg on Nov 25 2020, 9:47 AM.

Diff Detail

Event Timeline

csigg created this revision.Nov 25 2020, 9:47 AM
csigg requested review of this revision.Nov 25 2020, 9:47 AM

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.

herhut accepted this revision.Nov 27 2020, 1:04 AM

Cleaning this up in a follow-on is fine too. Please let @ftynse comment, too.

This revision is now accepted and ready to land.Nov 27 2020, 1:04 AM

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.

ftynse accepted this revision.Nov 27 2020, 11:06 AM

If you have time, maybe we can also consider passing in a SourceOp::Adaptor instead of raw operands?

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.

csigg updated this revision to Diff 308100.Nov 27 2020, 12:10 PM

Undo matchAndRewriter -> rewrite.