Page MenuHomePhabricator

[MLIR][Standard] Fix cast materialization for incompatible types
AcceptedPublic

Authored by frgossen on Aug 10 2020, 2:51 AM.

Details

Summary

Only create llvm.mlir.cast when types are cast compatible and fail otherwise.

Diff Detail

Event Timeline

frgossen created this revision.Aug 10 2020, 2:51 AM
Herald added a project: Restricted Project. · View Herald Transcript
frgossen requested review of this revision.Aug 10 2020, 2:51 AM
herhut added inline comments.Aug 10 2020, 3:00 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
174

This is addressing the FIXME below partially. A better way could be to expose the verifyMLIRCastType lambda from LLVMDialect::DialectCastOp::verify as a static function on LLVMDialect::DialectCastOp and call that here? Then the FIXME could disappear.

silvas added a subscriber: silvas.Aug 10 2020, 11:36 AM

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

The solution in this revision looks like a hack. Returning the input when a materialization is requested defeats the whole purpose of materializations, and only works because the assert isn't turned on yet. Realistically, the underlying issue can happen with any type right now. I have a proper fix in the pipeline that should solve this case, where the conversion infra removes casts that become redundant.

rriddle added inline comments.Aug 10 2020, 12:52 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
174

Agreed, ideally DialectCastOp should expose a static bool areCastCompatible(Type a, Type b); method like many of the other cast operations do. We could call that from here which would remove the TODO, but if that fails this method should fail to materialize.

The simultaneous application of patterns from SCF to standard and from standard to LLVM rewrites the following to invalid IR.

Do we actually need a simultaneous application?

The simultaneous application of patterns from SCF to standard and from standard to LLVM rewrites the following to invalid IR.

Do we actually need a simultaneous application?

We can also do this in steps but combining patterns should work. Or at least it should fail in a reasonable way and not produce invalid IR.

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

The solution in this revision looks like a hack. Returning the input when a materialization is requested defeats the whole purpose of materializations, and only works because the assert isn't turned on yet. Realistically, the underlying issue can happen with any type right now. I have a proper fix in the pipeline that should solve this case, where the conversion infra removes casts that become redundant.

Would the right thing be to return llvm::None to make this fail during conversion? The problem here is not that the cast is redundant but that the cast cannot be materialized as it is invalid.

Or should we extend the special cast operation such that it supports all casts as long as its operand and result types can be converted using the type converter?

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

The solution in this revision looks like a hack. Returning the input when a materialization is requested defeats the whole purpose of materializations, and only works because the assert isn't turned on yet. Realistically, the underlying issue can happen with any type right now. I have a proper fix in the pipeline that should solve this case, where the conversion infra removes casts that become redundant.

Would the right thing be to return llvm::None to make this fail during conversion? The problem here is not that the cast is redundant but that the cast cannot be materialized as it is invalid.

Yes, if a conversion can't be materialized this should return llvm::None.

Or should we extend the special cast operation such that it supports all casts as long as its operand and result types can be converted using the type converter?

Realistically, we probably want to be able to materialize casts for a majority(if not all) of the conversions that the default LLVM type converter provides. This makes it much easier to have progressive lowering, which is something we should strive to enable.

Realistically, we probably want to be able to materialize casts for a majority(if not all) of the conversions that the default LLVM type converter provides. This makes it much easier to have progressive lowering, which is something we should strive to enable.

Yes, and this should let us disentangle the existing whatever-to-LLVM from std-to-LLVM conversions.

frgossen updated this revision to Diff 286537.Aug 19 2020, 5:30 AM
frgossen marked 2 inline comments as done.

Address comments

frgossen added a comment.EditedAug 19 2020, 5:31 AM

Fully agree.

The mlir-cuda-runner tests are still failing because they materialized invalid casts. They did not fail because the verifier is not invoked before the lowering to LLVM (?) and the the lowering for llvm.mlir.cast is actually able to deal with the index case at least.

@rriddle, has the fix you mentioned landed yet? I could not find it.

frgossen retitled this revision from [MLIR][Standard] Fix cast materialization for index types to [MLIR][Standard] Fix cast materialization for incompatible types.Aug 19 2020, 5:35 AM
frgossen edited the summary of this revision. (Show Details)
frgossen added a reviewer: rriddle.
ftynse accepted this revision.Aug 19 2020, 6:49 AM
ftynse added inline comments.
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1019

Let's add a comment in addCastCompatible that asks to update the message in the verifier when more cases are added.

This revision is now accepted and ready to land.Aug 19 2020, 6:49 AM
frgossen updated this revision to Diff 286773.Aug 20 2020, 4:10 AM

Add comment

@rriddle, the mlir-cuda-runner tests are still failing because they materialize invalid casts. They did not fail earlier because there the verifier is not invoked when lowering to LLVM (?) and the the lowering for llvm.mlir.cast is actually able to deal with the index, it seems.

Has the fix you mentioned landed yet? I could not find it.

@rriddle, the mlir-cuda-runner tests are still failing because they materialize invalid casts. They did not fail earlier because there the verifier is not invoked when lowering to LLVM (?) and the the lowering for llvm.mlir.cast is actually able to deal with the index, it seems.

Has the fix you mentioned landed yet? I could not find it.

Unfortunately my attention has been pulled in another direction, and I likely won't have time to return to this at least for a few weeks.

@rriddle, fair enough. Can you let me know once it lands? I'm almost sure that fixes the failing tests here.
I will leave this CL until then to avoid duplicate work.

frgossen marked an inline comment as done.Aug 27 2020, 1:16 AM

Is there an update on this? Is this still relevant?

I believe this is still relevant as the FIXME is still present. The mlir-cuda-runner tests still fail.
@rriddle