This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add "mask" operand to vector.transfer_read/write.
ClosedPublic

Authored by springerm on Apr 6 2021, 6:29 PM.

Details

Summary

Also factors out out-of-bounds mask generation from vector.transfer_read/write into a new MaterializeTransferMask pattern.

Diff Detail

Event Timeline

springerm created this revision.Apr 6 2021, 6:29 PM
springerm requested review of this revision.Apr 6 2021, 6:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 6:29 PM
springerm updated this revision to Diff 335693.Apr 6 2021, 6:37 PM

Minor cleanup.

looks good, thanks for pushing on this !
Waiting for @ftynse to weigh in on the llvm.mlir.cast that became an index_cast and interplays with data layout.

mlir/test/Conversion/VectorToLLVM/vector-mask-to-llvm.mlir
7

why do you have this change ?
I am not 100% sure how this interplays with the recent data layout changes.
@ftynse ?

springerm updated this revision to Diff 335735.Apr 6 2021, 11:59 PM

Handle TransferReadToVectorLoadLowering correctly.

springerm updated this revision to Diff 335757.Apr 7 2021, 1:55 AM

Fix test.

ftynse accepted this revision.Apr 7 2021, 4:26 AM
ftynse added inline comments.
mlir/test/Conversion/VectorToLLVM/vector-mask-to-llvm.mlir
7

This is likely due to the change in the type of patterns used. Previously, VectorCreateMaskOpConversion was an ConvertOpToLLVMPattern and was processed by the infra for type mismatches/conversion, hence the llvm.milr.cast casting from index to i64. This change made it a regular RewritePattern, so there is no additional type handling. Inside the pattern, createCastToIndexLike gets called and emits index_cast instead of trunci because the operand is no longer casted by the infra and remains index.

I don't think data layout has any effect here. llvm.mlir.cast casts to the index type with the bitwidth specified by the data layout (64 by default). createCastToIndexLike does nothing if source/target type match, or inserts a trunci/sext otherwise. Assuming index bitwidth is set to 32, the old code would have an llvm.mlir.cast to i32 and skip the no-longer-necessary trunci; the new code still has the index_cast that would become a noop after lowering. Assuming index bitwidth is set to 64, llvm.mlir.cast to i64 would become a noop, and index_cast would become a trunci leading to exactly the same code.

This revision is now accepted and ready to land.Apr 7 2021, 4:26 AM
springerm added inline comments.Apr 7 2021, 4:31 AM
mlir/test/Conversion/VectorToLLVM/vector-mask-to-llvm.mlir
7

I think this is because the original VectorCreateMaskOpConversion pattern was a ConvertOpToLLVMPattern. As such, the operand was type-converted from index to i64 and the lowering pattern then truncates i64 to i32 (enableIndexOptimizations).

The new VectorCreateMaskOpConversion is no longer a ConvertOpToLLVMPattern and retains the index type. The pattern index_casts the index directly to i32.

This casting logic is implemented in createCastToIndexLike (unchanged, just moved it to another file). Anyway both should be identical. ("If casting to a narrower integer, the value is truncated.") I don't think it will cause any problems.

springerm updated this revision to Diff 335780.Apr 7 2021, 4:59 AM

Minor cleanups.

springerm updated this revision to Diff 335782.Apr 7 2021, 5:12 AM

Add additional builder required for integration.

This revision was landed with ongoing or failed builds.Apr 7 2021, 5:33 AM
This revision was automatically updated to reflect the committed changes.