Page MenuHomePhabricator

[mlir:DialectConversion] Restructure how argument/target materializations get invoked
ClosedPublic

Authored by rriddle on Oct 12 2021, 2:01 AM.

Details

Summary

The current implementation invokes materializations
whenever an input operand does not have a mapping for the
desired type, i.e. it requires materialization at the earliest possible
point. This conflicts with goal of dialect conversion (and also the
current documentation) which states that a materialization is only
required if the materialization is supposed to persist after the
conversion process has finished.

This revision refactors this such that whenever a target
materialization "might" be necessary, we insert an
unrealized_conversion_cast to act as a temporary materialization.
This allows for deferring the invocation of the user
materialization hooks until the end of the conversion process,
where we actually have a better sense if it's actually
necessary. This has several benefits:

  • In some cases a target materialization hook is no longer necessary

When performing a full conversion, there are some situations
where a temporary materialization is necessary. Moving forward,
these users won't need to provide any target materializations,
as the temporary materializations do not require the user to
provide materialization hooks.

  • getRemappedValue can now handle values that haven't been converted yet

Before this commit, it wasn't well supported to get the remapped
value of a value that hadn't been converted yet (making it
difficult/impossible to convert multiple operations in many
situations). This commit updates getRemappedValue to properly
handle this case by inserting temporary materializations when
necessary.

Another code-health related benefit is that with this change we
can move a majority of the complexity related to materializations
to the end of the conversion process, instead of handling adhoc
while conversion is happening.

Diff Detail

Event Timeline

rriddle created this revision.Oct 12 2021, 2:01 AM
rriddle requested review of this revision.Oct 12 2021, 2:01 AM
ftynse accepted this revision.Oct 18 2021, 5:39 AM

LGTM!

It would be nice to have tests for cases where unrealized_conversion_cast is already present in the input IR.

mlir/include/mlir/IR/BlockAndValueMapping.h
93–94

Would you mind commenting why preserving the order of insertion is important here?

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
313

Nice catch! I was thinking about some automated way to catch op/adaptor misuses, and it looks like this change is making them break early.

mlir/lib/Transforms/Utils/DialectConversion.cpp
372

Nit: "illegal type" -> "op result of illegal type". Argument materialization is also technically a materialization of an illegal type.

455

Nit: I'm slightly concerned about the asymmetry between buildUnresolvedArgumentMaterialization using the type converter to obtain the output type and buildUnresolvedTargetMaterialization just taking it as argument (which is computed by its only caller using the type converter).

745

Nit: drop "we".

1068

How about adding some debug output in this case so we can actually see which patterns rely on this?

1271–1272

Nit: it's not necessarily an operand anymore

2830

Nit: sink the declaration into the loop?

2840

This deserves a comment as to why we are erasing there (IIUC, the mapping has become circular, which means we should be erasing it completely).

2872

Nit: s/affect/effect

mlir/test/Transforms/test-legalize-remapped-value.mlir
29

Nit: add the newline plz

This revision is now accepted and ready to land.Oct 18 2021, 5:39 AM
rriddle updated this revision to Diff 381409.Oct 21 2021, 3:16 PM
rriddle marked 10 inline comments as done.

Address comments

Thanks for the review Alex!

mlir/include/mlir/IR/BlockAndValueMapping.h
93–94

Nice catch! This was necessary in a previous version of the patch, but the order isn't important anymore.

mlir/lib/Transforms/Utils/DialectConversion.cpp
372

Target materializations can also be applied to arguments though. Argument materializations shouldn't be used to materialize illegal types AFAIK, they should only be used to materialize legal types. That being said, the way the hook is written today does not make that clear (given that you pass in the illegal type as the expected result type, even though all of our cases today just use that to decide which legal type to actually produce).

I think our current documentation for these are fairly confusing, or just lacking in general. The way they work right now is that Argument materialization is an optional hook that effectively enables 2-stage conversion/materialization for block arguments; whereas Target materialization is the general/default "convert this illegal type into a legal one".

1068

Nice idea! Added some logging when we detect this when legalizing changed results.

This revision was landed with ongoing or failed builds.Oct 26 2021, 7:12 PM
This revision was automatically updated to reflect the committed changes.

The broken flang test should be fixed by rG42831686034b.

silvas added a subscriber: silvas.Oct 26 2021, 10:28 PM

Glad to see this happening!

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

The FIXME is more of further elaborating on the existing state, it was not some new problem added by this patch. Can you file a bug report with what your exact situation is?

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

The FIXME is more of further elaborating on the existing state, it was not some new problem added by this patch. Can you file a bug report with what your exact situation is?

One change that was made is that inserted materializations aren't legalized anymore, but that was mostly prevent situations where that legalization doesn't mesh well with the fact that the conversion is being finalized (we would have to recursively finalize in some capacity). If that behavior is necessary, it could potentially be re-added in some limited capacity. The way it was before was extremely fragile though.

silvas added a comment.Nov 2 2021, 4:49 PM

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

The FIXME is more of further elaborating on the existing state, it was not some new problem added by this patch. Can you file a bug report with what your exact situation is?

I suspect it is that we can't pattern-match the unrealized_conversion_cast ops the way we used to be able to (when they were our materializations). Previously it was possible to use the inserted cast as a marker indicating a "bundle" of operands in 1->N conversion. We now don't have a way to represent that "bundle" concept.

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

The FIXME is more of further elaborating on the existing state, it was not some new problem added by this patch. Can you file a bug report with what your exact situation is?

I suspect it is that we can't pattern-match the unrealized_conversion_cast ops the way we used to be able to (when they were our materializations). Previously it was possible to use the inserted cast as a marker indicating a "bundle" of operands in 1->N conversion. We now don't have a way to represent that "bundle" concept.

Right, 1->N conversions are not something I would consider "well supported" and never have been. Part of the point of this patch is to pave a way where that isn't the case. I've got a CL in my stack that adds "builtin" 1->N conversions (i.e. replaceOp "just works", utilities for expanding the temporary conversions, etc.). That ideally should remove the need to explicitly match any operation when expanding, given that dialect conversion knows exactly which operations are the temporary "bundles" and which aren't (given that dialect conversion inserted them in the first place). User operations should only need to be created via the existing materialization hooks and only when necessary (i.e. uses remain at the end of conversion).