Page MenuHomePhabricator

[mlir][DialectConversion] Enable deeper integration of type conversions
ClosedPublic

Authored by rriddle on Jun 29 2020, 5:56 PM.

Details

Summary

This revision adds support for much deeper type conversion integration into the conversion process, and enables auto-generating cast operations when necessary. Type conversions are now largely automatically managed by the conversion infra when using a ConversionPattern with a provided TypeConverter. This removes the need for patterns to do type cast wrapping themselves and moves the burden to the infra. This makes it much easier to perform partial lowerings when type conversions are involved, as any lingering type conversions will be automatically resolved/legalized by the conversion infra.

To support this new integration, a few changes have been made to the type materialization API on TypeConverter. Materialization has been split into three separate categories:

  • Argument Materialization: This type of materialization is used when converting the type of block arguments when calling convertRegionTypes. This is useful for contextually inserting additional conversion operations when converting a block argument type, such as when converting the types of a function signature.
  • Source Materialization: This type of materialization is used to convert a legal type of the converter into a non-legal type, generally a source type. This may be called when uses of a non-legal type persist after the conversion process has finished.
  • Target Materialization: This type of materialization is used to convert a non-legal, or source, type into a legal, or target, type. This type of materialization is used when applying a pattern on an operation, but the types of the operands have not yet been converted.

Depends On D82830

Diff Detail

Event Timeline

rriddle created this revision.Jun 29 2020, 5:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:56 PM

@ftynse I still need to add tests and rewrite the documentation, but it should be good to start looking over. Thanks.

ftynse added a comment.Jul 2 2020, 3:42 AM

Thanks, River! Looks good in general. Two high-level questions:

  1. What is the convention on inserting materializations, i.e. who is in charge of inserting one, at which point in the conversion and using which type converter? For example, legalizeChangedResultType checks with the type converter of the user (which it takes from opToConverter, so it is only available for ops created by the infra), but attempts materialization with replConverter that it accepts as an argument, and which may be different.
  2. This looks like it can get into infinite recursion or iteration if materialization produces operations that need more materialization in order to be legalized. Is there something the infra does to guard against it?
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
179

Hmm, can we already call this->convertType() from here? DialectCastOp should be constructible if convertType(resultType) == inputs[0] for source conversions and if convertType(inputs[0]) == resultType for target conversions. I was thinking about how one can check this in the verifier of DialectCastOp, but this may actually be a better place.

mlir/lib/Transforms/DialectConversion.cpp
110–118

Nit: spurious backslash

409

Nit: might be useful to attach a note pointing to the first live user

602

Initialize to nullptr?

1015

assert(legalTypes.size() <= 1) then?

1021

I'm not sure I read this correctly. A lot (all?) patterns rely on receiving new operands to create new operations, which require Values (sometimes, of some specific type) to be passed in. Is the idea here to fail the conversion if there is a type mismatch, no type converter is provided, and we can't find a mapping to the desired (original) type?

2327

What if the user doesn't have a converter, and the types don't match?

rriddle updated this revision to Diff 277119.Jul 10 2020, 12:14 PM
rriddle marked 11 inline comments as done.

Resolve comments and add tests

rriddle added inline comments.Jul 10 2020, 12:14 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
179

I don't think we can given that users of the type converter can add other type casts, so we aren't guaranteed to have just standard type conversions. At least, that's if we keep DialectCastOp solely between Standard and LLVM.

mlir/lib/Transforms/DialectConversion.cpp
1015

We don't want to assert, because that would prevent 1->N conversions outright. There isn't anything in the infra to detect/protect against it, so the best we can do is what we have been doing; i.e. you need to manage it yourself.

1021

Essentially yes. In the new ideal state, patterns that rely on type conversions must provide a TypeConverter. If one isn't provided, we would always use the original type and rely on the infra for making sure lingering type materializations are inserted when necessary.

2327

Thanks for the catch, refactored to remove the need for this.

Thanks, River! Looks good in general. Two high-level questions:

  1. What is the convention on inserting materializations, i.e. who is in charge of inserting one, at which point in the conversion and using which type converter? For example, legalizeChangedResultType checks with the type converter of the user (which it takes from opToConverter, so it is only available for ops created by the infra), but attempts materialization with replConverter that it accepts as an argument, and which may be different.

This has been reworked a bit, but ideally only the pattern that replaced the operation should have its type converter used/checked. We are in a bit of an awkward state where a lot of patterns rely on broken behavior w.r.t. type conversions. I was able to remove the checks for user type converters, and there are a few more TODOs sprinkled about to slowly enable more aggressive checks as user patterns get updated.

  1. This looks like it can get into infinite recursion or iteration if materialization produces operations that need more materialization in order to be legalized. Is there something the infra does to guard against it?

There is nothing guarding it in this revision, but there should be in a followup. Realistically a generated conversion should be already legal, or trivially legalizable. I'm still playing around with if we should check if it is legal without trying to legalize, but wanted to update more patterns first to decide if that is the desired behavior.

ftynse accepted this revision.Jul 23 2020, 5:10 AM

Thanks, and apologies for the delay! Phabricator wasn't showing this on the front page for some reason (this happened to me in the past, any ideas about the reason for this weird behavior is most welcome) so I missed it. Don't hesitate to ping me if I'm not responding for more than several work days.

This looks much easier to follow after the refactoring! Two more high-level comments, feel free to leave for follow-ups:

  • Would it make sense to sprinkle some debug output that connects with the existing output for pattern matches/failures and op creation in patterns?
  • Could we start a publicly visible doc that has the list of current assumptions the conversion infra makes, and whether we want to remove some of them? There are out-of-tree users that may rely on some of the deprecated behavior and it will be easier to point them to that than to a diff.
mlir/lib/IR/Value.cpp
82

Or maybe blocks? It has always bothered me that we report something like "block with no terminator" while pointing to something else (parent op or first op in the block)

mlir/lib/Transforms/DialectConversion.cpp
2187

Typo: operations

This revision is now accepted and ready to land.Jul 23 2020, 5:10 AM
rriddle updated this revision to Diff 280316.Jul 23 2020, 7:38 PM
rriddle marked an inline comment as done.

Resolve comments

This revision was automatically updated to reflect the committed changes.

Thanks, and apologies for the delay! Phabricator wasn't showing this on the front page for some reason (this happened to me in the past, any ideas about the reason for this weird behavior is most welcome) so I missed it. Don't hesitate to ping me if I'm not responding for more than several work days.

No worries, I had plenty of other things to do.

This looks much easier to follow after the refactoring! Two more high-level comments, feel free to leave for follow-ups:

  • Would it make sense to sprinkle some debug output that connects with the existing output for pattern matches/failures and op creation in patterns?

I added in a few, but should probably add more for the things that are currently marked with TODOs.

  • Could we start a publicly visible doc that has the list of current assumptions the conversion infra makes, and whether we want to remove some of them? There are out-of-tree users that may rely on some of the deprecated behavior and it will be easier to point them to that than to a diff.

I have an in progress rewrite of the DialectConversion doc that should touch on many of these things. I'm going to try and have that out for review early next week, it'll have a section for the deprecated and soon to be replaced behavior.

Thanks again for the review Alex!