This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Replace the materializeCallConversion inlining hook.
AbandonedPublic

Authored by gysit on Mar 12 2023, 8:31 AM.

Details

Summary

This revision removes the materializeCallConversion hook and replaces
it by a isTypeConvertible hook that checks if an argument or a result
conversion is possible. The actual conversion is then performed using
the recently introduced handleArgument and handleResult hooks.

The revision enables argument and result conversions that create
multiple operations rather than a single cast operation. Additionally,
the handlers also have access to argument and result attributes that
may provide extra information such as if an integer has to be sign or
zero extended.

Depends on D145582

Diff Detail

Event Timeline

gysit created this revision.Mar 12 2023, 8:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 12 2023, 8:31 AM
gysit requested review of this revision.Mar 12 2023, 8:31 AM
Dinistro accepted this revision.Mar 14 2023, 2:22 AM

I only have one small comment apart from that this LGTM.

mlir/lib/Transforms/Utils/InliningUtils.cpp
493

NIT: you can directly return the result of this function.

This revision is now accepted and ready to land.Mar 14 2023, 2:22 AM
gysit updated this revision to Diff 505004.Mar 14 2023, 2:59 AM

Address review comment.

gysit marked an inline comment as done.
gysit updated this revision to Diff 506966.Mar 21 2023, 7:10 AM

Rebase and extrapolate review comments.

gysit updated this revision to Diff 507340.Mar 22 2023, 6:30 AM

Rebase and remove and remove flang type conversion logic.

gysit added a comment.Mar 22 2023, 7:04 AM

This revision uses the argument and result handlers, introduced in https://reviews.llvm.org/D145582, to implement type conversion. One side-effect of this change is that the dialect interface is selected based on the callable rather than based on the call operation. The reason for this change is that the new handlers access to the argument and result attributes that are stored on the callable and not on the call. Selecting the dialect interface based on the callable makes more sense. This impacts Flang's inliner implementation since the FIR dialect implements its own call operation but does not implement a custom function operation. As a result, it is not really possible to implement FIR-specific type conversions with the new approach (except by implementing an FIR function operation).

@clementval would it be ok to temporarily drop the materializeCallConversion without a replacement (I did not find a test that exercises the type conversion logic)? Once the FIR dialect has a custom function operation type conversions would be possible again.

Alternatively, we may also return to selecting the dialect interface based on the call operation. I am not fully convinced this is a good idea since the argument and result attributes really are a property of the callable/function.

@rriddle do you have a strong preference here?

gysit abandoned this revision.Apr 5 2023, 5:37 AM

We decided to abandon this revision in favor of https://reviews.llvm.org/D147605 which is simpler and satisfies our requirements. The tricky part with this revision was that we may not call the type conversion hook based on the callable dialect since existing downstream implementations (and flang) perform the type conversion based on the call dialect.