This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector] Add an optional "masked" boolean array attribute to vector transfer operations
ClosedPublic

Authored by nicolasvasilache on May 17 2020, 6:26 PM.

Details

Summary

Vector transfer ops semantic is extended to allow specifying a per-dimension masked
attribute. When the attribute is false on a particular dimension, lowering to LLVM emits
unmasked load and store operations.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2020, 6:26 PM
ftynse accepted this revision.May 18 2020, 6:11 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
1124

The comment looks wrong given that the builder takes a permutationMap

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
761–762

I wonder if we actually need template specialization here, or just overloading the function on the operation type would have sufficed?

mlir/lib/Dialect/Vector/VectorOps.cpp
1305

It's already an ArrayAttr, no need to cast

1308

Nit: "requires optional" sounds weird to me. "expects the optional 'masked' attr to be of <...> rank when present"?

1356

Isn't it already an ArrayAttr? In any case, I think you should just cast because the verifier ensures it's an array attribute.

This revision is now accepted and ready to land.May 18 2020, 6:11 AM
nicolasvasilache marked 5 inline comments as done.

Address review.

nicolasvasilache planned changes to this revision.May 18 2020, 8:03 AM

Messed the rebase, will revisit.

This revision is now accepted and ready to land.May 18 2020, 8:50 AM
nicolasvasilache accepted this revision.May 18 2020, 8:51 AM

Rebased properly.

This revision was automatically updated to reflect the committed changes.

Thanks, Nicolas! It looks good! Just a minor comment below.
This patch introduces an attribute to set if a vector load/store is masked but how is that mask being computed/passed to the vector load/store?
IIUC, the current implementation assumes that there is no divergent control flow and therefore the loop exit condition can be used as mask for all the vector loads/stores, is that correct?

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
923

I think we should make padding optional and avoid it when masked is false. It doesn't make sense, right?