This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Implement pass utils for 1:N type conversions.
ClosedPublic

Authored by ingomueller-net on Feb 21 2023, 3:35 AM.

Details

Summary

The current dialect conversion does not support 1:N type conversions.
This commit implements a (poor-man's) dialect conversion pass that does
just that. To keep the pass independent of the "real" dialect conversion
infrastructure, it provides a specialization of the TypeConverter class
that allows for N:1 target materializations, a specialization of the
RewritePattern and PatternRewriter classes that automatically add
appropriate unrealized casts supporting 1:N type conversions and provide
converted operands for implementing subclasses, and a conversion driver
that applies the provided patterns and replaces the unrealized casts
that haven't folded away with user-provided materializations.

The current pass is powerful enough to express many existing manual
solutions for 1:N type conversions or extend transforms that previously
didn't support them, out of which this patch implements call graph type
decomposition (which is currently implemented with a ValueDecomposer
that is only used there).

The goal of this pass is to illustrate the effect that 1:N type
conversions could have, gain experience in how patterns should be
written that achieve that effect, and get feedback on how the APIs of
the dialect conversion should be extended or changed to support such
patterns. The hope is that the "real" dialect conversion eventually
supports such patterns, at which point, this pass could be removed
again.

Diff Detail

Event Timeline

ingomueller-net added a subscriber: ftynse.

Backport improvements in response to a review from @ftynse made here: https://github.com/iree-org/iree-llvm-sandbox/pull/655.

ingomueller-net published this revision for review.Mar 6 2023, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 7:05 AM

Add missing changes in last revision. (I messed up using patch to port the changes from one repo to the other.)

Remove SCF conversions and tests to avoid distractions.

ingomueller-net edited the summary of this revision. (Show Details)Mar 7 2023, 2:03 AM

Improve comments for test patterns and test pass.

I have also implemented a decomposition of complex<T> to T, T and extended the test cases of StandardToComplex to run the decomposition either before or after that pass here: https://reviews.llvm.org/D145501. The fact both order produce the same result shows that the 1:N type conversion composes cleanly with other transforms.

Mogball added inline comments.Mar 7 2023, 6:48 AM
mlir/test/lib/Conversion/OneToNTypeConversion/TestOneToNTypeConversionPass.cpp
22

Test passes should be define in mlir/test/lib/

mlir/test/lib/Conversion/OneToNTypeConversion/TestOneToNTypeConversionPass.cpp
22

I don't understand. Isn't that the path were the pass is defined already?

Add diagnostics when user materialization fails.

ingomueller-net retitled this revision from Implement test pass for 1:N type conversions. to [mlir] Implement test pass for 1:N type conversions..Mar 20 2023, 2:46 AM

Minor: fix order of CHECK matches in test case.

A few minor updates back-ported from https://github.com/iree-org/iree-llvm-sandbox/pull/655:

  • Replace SmallVector<Value> by ValueRange in more parts of the API.
  • Use ValueRange instead of SmallVector for a few temporaries.
  • Fix typo in comment (NFC).

After talking to @ftynse, I am moving the main implementation back out of test into Transforms/Utils.

Also fixed an error about an global constructor/destructor.

I have made several passes over the patch with @ftynse. I believe we came to the conclusion that current patch is in line with the discussion on discourse and more or less ready to land. I'll wait a few more days for anybody else to object or suggest further improvements.

springerm accepted this revision.Mar 20 2023, 6:08 AM
springerm added inline comments.
mlir/include/mlir/Dialect/Func/Transforms/OneToNFuncConversions.h
20 ↗(On Diff #506545)

nit: put in backticks

mlir/include/mlir/Transforms/OneToNTypeConversion.h
12 ↗(On Diff #506545)

what the "main" function?

42 ↗(On Diff #506545)

nit: Add documentation for this this callback, e.g., when is it used.

55 ↗(On Diff #506545)

Can there be multiple materializations? What happens in that case? Is this consistent with the 1:1 type converter?

69 ↗(On Diff #506545)

nit: I've seen mostly uses without a size hint recently.

186 ↗(On Diff #506545)

typo

187 ↗(On Diff #506545)

function has LogicalResult return type

189 ↗(On Diff #506545)

typo

191 ↗(On Diff #506545)

do you mean the caller?

241 ↗(On Diff #506545)

I guess this is a partial conversion? In that case let's call it applyPartialOneToNConversion to be consistent with applyPartialConversion.

mlir/lib/Dialect/Func/Transforms/OneToNFuncConversions.cpp
24 ↗(On Diff #506545)

Wrap all conversion patterns in an anonymous namespace.

45 ↗(On Diff #506545)

Is this needed?

mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp
105–106 ↗(On Diff #506545)

Attributes should not be leaked across pass boundaries. Does the resulting IR have such attributes? Can they be removed at the end of the dialect conversion?

Could this be done without attributes, e.g., storing the cast operations in a SmallVector in the type converter etc.?

131 ↗(On Diff #506545)

casts

This revision is now accepted and ready to land.Mar 20 2023, 6:08 AM
ingomueller-net marked 13 inline comments as done.Mar 20 2023, 7:23 AM

Thanks a lot for the thorough feedback, @springerm! Will push an updated revision in one second.

mlir/include/mlir/Dialect/Func/Transforms/OneToNFuncConversions.h
20 ↗(On Diff #506545)

Good catch! Fixed here and in all other places I found I had them missing.

mlir/include/mlir/Transforms/OneToNTypeConversion.h
12 ↗(On Diff #506545)

Yeah, that needs explanation. Fixed!

42 ↗(On Diff #506545)

Yep! Added!

55 ↗(On Diff #506545)

There can be many and what happens was eluded to with "in LIFO order". In the last revision, I elaborate more on what that means.

69 ↗(On Diff #506545)

Good point! I have asked myself what the best practice is here for a while... turns out the developer guide suggests to not specify a size if you don't have a good argument for a particular value.

187 ↗(On Diff #506545)

Excellent catch! The comment was out of date: I had implemented the conversion back to the original types through a return value originally but found a more elegant way through sub-classing PatternRewriter and adding an overload there that does that. The newest revision has an updated (and shorter) comment.

191 ↗(On Diff #506545)

(removed in latest revision)

241 ↗(On Diff #506545)

Yep, good suggestion.

I also rewrote the documentation for this function to explain in a more self-contained way what the function does (and why it is called "partial").

mlir/lib/Dialect/Func/Transforms/OneToNFuncConversions.cpp
24 ↗(On Diff #506545)

Thanks for the hint! I wasn't aware of the issue for classes, but I guess it makes sense...

45 ↗(On Diff #506545)

Nope :) Fixed!

mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp
105–106 ↗(On Diff #506545)

I am pretty sure they can't: applyPartialOneToNConversion walks over all UnrealizedConversionCasts and replaces those with the attribute named like castKindAttrName with user materializations. All those casts that are inserted with such an attributed are thus replaced before the end of the pass.

In an earlier version, I had collected all cast ops before doing any changes but because these changes may remove some of the original cast ops, I'd have dangling pointers.

ingomueller-net marked 10 inline comments as done.

Address issues raised by @springerm. Thanks a lot for the thorough feedback!

Rebasing to latest main.

ingomueller-net retitled this revision from [mlir] Implement test pass for 1:N type conversions. to [mlir] Implement pass utils for 1:N type conversions..Mar 21 2023, 1:24 AM
ingomueller-net reopened this revision.Mar 27 2023, 5:54 AM
This revision is now accepted and ready to land.Mar 27 2023, 5:54 AM

Adding build dependencies for Bazel builds to overlay.

(Trying to make Phabricator do what I want...)

Why the revert? (please mention the revert reason in the commit message when reverting).

Thanks for the question and the hint. This is my first big commit and first revert :S

The Bazel build broke because I added new dependencies in CMake but not in the Bazel overlay (or whatever it's called exactly). The tests of the revision are now running (after they were stuck due to some CI problems for some time) but should finish any time. Then I'll land again.

Bazel is not a supported part of the project: it can't be a cause for revert as far as I am concerned.

I followed a suggestion from folks at Google who are responsible for the tests that failed there.

What should I do now?

It's okay to reland.

(Rebasing as Windows build was broken due to unrelated problem.)

I followed a suggestion from folks at Google who are responsible for the tests that failed there.

Yes I figured: this wasn't good advice: downstream problems are to be handled downstream (even if it means reverting a commit downstream as needed).

What should I do now?

You can re-land as Alex mentioned, my comment was forward looking and FYI.

Yes I figured: this wasn't good advice: downstream problems are to be handled downstream (even if it means reverting a commit downstream as needed).

OK, I think I understand the policy now and follow it in the future. Thanks!

You can re-land as Alex mentioned, my comment was forward looking and FYI.

OK, thanks! Done :)