Page MenuHomePhabricator

[mlir][DialectConversion] Refactor how block argument types get converted
ClosedPublic

Authored by rriddle on Thu, Jun 11, 12:05 PM.

Details

Summary

This revision removes the TypeConverter parameter passed to the apply* methods, and instead moves the responsibility of region type conversion to patterns. The types of a region can be converted using the 'convertRegionTypes' method, which acts similarly to the existing 'applySignatureConversion'. This method ensures that all blocks within, and including those moved into, a region will have the block argument types converted using the provided converter.

This has the benefit of making more of the legalization logic controlled by patterns, instead of being handled explicitly by the driver. It also opens up the possibility to support multiple type conversions at some point in the future.

This revision also adds a new utility class FailureOr<T> that provides a LogicalResult friendly facility for returning a failure or a valid result value.

Depends On D81680

Diff Detail

Event Timeline

rriddle created this revision.Thu, Jun 11, 12:05 PM
rriddle updated this revision to Diff 270914.Mon, Jun 15, 4:46 PM

Update doc to mention convertRegionTypes.

Can you update the doc as well for the apply* replacement: https://mlir.llvm.org/docs/DialectConversion/#region-signature-conversion

Done. Type conversion documentation in general should be getting a revamp over the coming weeks.

ftynse accepted this revision.Tue, Jun 16, 2:58 AM
ftynse added inline comments.
mlir/include/mlir/Transforms/DialectConversion.h
309

Could we initialize it to nullptr?

mlir/lib/Transforms/DialectConversion.cpp
220–221

Nit: this does not seem to return nullptr anymore

227–231

Nit: could you please update the doc to say why converter is necessary, in addition to singatureConversion (it is used to materialize casts, and stored for eventual undo).

368

Detached blocks are assumed to be converted? Or deleted?

371

Nit: I don't understand what is a "parent converter". Is this left over from some previous version that was looking up the converter in regionToConverter (which would also explain the check for the block to have a parent)

610

API Nit: this takes TypeConverter * but convrtRegionTypes below takes TypeConverter &. Would be nice to have them homogenized.

1582

Nit: operationsToIgnore ?

This revision is now accepted and ready to land.Tue, Jun 16, 2:58 AM
This revision was automatically updated to reflect the committed changes.
rriddle marked 8 inline comments as done.
rriddle added inline comments.Thu, Jun 18, 4:29 PM
mlir/include/mlir/Transforms/DialectConversion.h
309

Ooops, thanks for the catch.