This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Support FuncOpSignatureConversion for more FunctionLike ops.
ClosedPublic

Authored by mikeurbach on Jan 19 2021, 9:40 PM.

Details

Summary

This extracts the implementation of getType, setType, and getBody from
FunctionSupport.h into the mlir::impl namespace and defines them
generically in FunctionSupport.cpp. This allows them to be used
elsewhere for any FunctionLike ops that use FunctionType for their
type signature.

Using the new helpers, FuncOpSignatureConversion is generalized to
work with all such FunctionLike ops. Convenience helpers are added to
configure the pattern for a given concrete FunctionLike op type.

Diff Detail

Event Timeline

mikeurbach created this revision.Jan 19 2021, 9:40 PM
mikeurbach requested review of this revision.Jan 19 2021, 9:40 PM

Remove unrelated change.

This is the patch discussed with @silvas in https://llvm.discourse.group/t/bufferization-framework-replacement/2446/7. I proceeded here along the same lines as when I wanted to generalize the eraseArguments and eraseResults helpers from FuncOp to FunctionLike, but perhaps there is a better way to achieve this. CC'ing @rriddle and @ftynse who had helpful feedback when I was working on FunctionSupport.h before.

rriddle added inline comments.Jan 19 2021, 10:00 PM
mlir/lib/Transforms/Utils/DialectConversion.cpp
2524

This doesn't generalize to all FunctionLike operations though, just the ones that are similar to FuncOp (e.g. this would crash LLVMFuncOp). I'm +1 on having a simple default implementation that other FunctionLike operations can hook into, but I don't think it needs to match all of them. Could we instead just make it as simple as having the user provide the op name of the function like operation they want to use here?

ftynse added inline comments.Jan 20 2021, 12:33 AM
mlir/lib/Transforms/Utils/DialectConversion.cpp
2524

I would just make it a template <OpTy> struct FuncLikeSignatureConversion : public ConversionPattern<OpTy>. The class will probably move to the header, but we can keep the actual implementation in the impl namespace.

Switch to parameterizing the pattern by op type.

mikeurbach retitled this revision from [mlir] Generalize FuncOpSignatureConversion to all FunctionLike ops. to [mlir] Generalize FuncOpSignatureConversion for FunctionLike ops..
mikeurbach edited the summary of this revision. (Show Details)

Update revision description to match commit message.

Remove unused include.

rriddle added inline comments.Jan 20 2021, 4:21 PM
mlir/include/mlir/Transforms/DialectConversion.h
832

I don't think you need to expose the pattern. Just the operation name should be enough, which is all that OpConversionPattern does for you.

mikeurbach added inline comments.Jan 20 2021, 4:32 PM
mlir/lib/Transforms/Utils/DialectConversion.cpp
2524

Thanks for the feedback. I think I have updated the implementation along the lines Alex mentioned. Please let me know what you think of this approach.

mikeurbach added inline comments.Jan 20 2021, 4:35 PM
mlir/include/mlir/Transforms/DialectConversion.h
832

Ah ok I wasn't sure about this. I will try it out.

mikeurbach added inline comments.Jan 20 2021, 6:03 PM
mlir/include/mlir/Transforms/DialectConversion.h
832

@rriddle maybe I am misunderstanding, but I tried re-working this, and I am not sure how it should work. When you say don't need to expose the pattern, do you mean the pattern can be declared in the cpp file instead of this header?

I tried putting the pattern back in the cpp file, and then I had to update the populateFunctionLikeTypeConversionPattern helper that I call from outside MLIR. This templated helper needs to be defined in this header, correct?

I tried just forward declaring the pattern so I could use it in populateFunctionLikeTypeConversionPattern, but there are some enable_ifs that need to know about the base class, and other compile time assumptions about the pattern's constructor. So then I tried declaring the pattern here in the header, but defining it in the cpp file, and that failed with an error like so:

ld.lld: error: undefined symbol: mlir::FunctionLikeSignatureConversion<circt::handshake::FuncOp>::matchAndRewrite(circt::handshake::FuncOp, llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const

It seems like I need to define the templated matchAndRewrite here in the header as well. If I do that, and shove as much as I can into the impl namespace in the cpp file, that is basically what is in the current revision.

Does this sound reasonable or am I missing something? I think this is what Alex was getting at previously with the comment "The class will probably move to the header". Maybe I am missing the point here, and there is a better way to do this... Thanks for bearing with me.

rriddle added inline comments.Jan 20 2021, 6:13 PM
mlir/include/mlir/Transforms/DialectConversion.h
832

Apologies for the terse response, kept getting distracted by other things. What I mean is that OpConversionPattern is just a utility wrapper around ConversionPattern, that only serves to provide the name of the operation. What I was suggesting is that we expose the minimal amount necessary, which is really just what operation we are matching given that the things needed for the transformation already have generic utilities available(e.g. getting/setting the function's type). For example, could we do something like:

// Header file

void populateFuncOpTypeConversionPattern(StringRef functionLikeOpName,
                                         OwningRewritePatternList &patterns,
                                         MLIRContext *ctx,
                                         TypeConverter &converter);

template <typename FuncOpT>
void populateFuncOpTypeConversionPattern(OwningRewritePatternList &patterns,
                                         MLIRContext *ctx,
                                         TypeConverter &converter) {
  populateFuncOpTypeConversionPattern(FuncOpT::getOperationName(), patterns, ctx, converter);
}

// Source File

namespace {
struct FuncOpSignatureConversion : public ConversionPattern {
  FuncOpSignatureConversion(StringRef funcOpName, MLIRContext *ctx, TypeConverter &converter)
      : ConversionPattern(funcOpName, /*benefit=*/1, converter, ctx) {}

  LogicalResult
  matchAndRewrite(Operation *op, ArrayRef<Value> operands,
                  ConversionPatternRewriter &rewriter) const override {
    // Implementation
  }
};
} // end anonymous namespace

void mlir::populateFuncOpTypeConversionPattern(
    StringRef functionLikeOpName, OwningRewritePatternList &patterns, MLIRContext *ctx,
    TypeConverter &converter) {
  patterns.insert<FuncOpSignatureConversion>(functionLikeOpName, ctx, converter);
}

?

mikeurbach added inline comments.Jan 20 2021, 6:27 PM
mlir/include/mlir/Transforms/DialectConversion.h
832

Ah I see, thank you that is very helpful. I do think we can go that route.

Updates after most recent feedback.

mikeurbach marked 3 inline comments as done.

Clean up the diff a bit.

mikeurbach retitled this revision from [mlir] Generalize FuncOpSignatureConversion for FunctionLike ops. to [mlir] Support FuncOpSignatureConversion for more FunctionLike ops..
mikeurbach edited the summary of this revision. (Show Details)

Update title and description to match commit message.

mikeurbach edited the summary of this revision. (Show Details)

Fix typo in commit message.

Harbormaster completed remote builds in B86037: Diff 318105.
rriddle accepted this revision.Jan 21 2021, 3:36 PM

LGTM. Thanks!

mlir/include/mlir/Transforms/DialectConversion.h
425

I would add a note that this only works for FunctionLike operations that use FunctionType to represent their type.

This revision is now accepted and ready to land.Jan 21 2021, 3:36 PM

Add note that the new functionality only supports FunctionLike ops that use FunctionType for their signature.

mikeurbach marked 2 inline comments as done.Jan 21 2021, 3:48 PM
This revision was landed with ongoing or failed builds.Jan 21 2021, 5:35 PM
This revision was automatically updated to reflect the committed changes.

Thanks @rriddle for the iterations on this. Looking forward to using this in CIRCT with our FunctionLike ops.