This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Standard] Make the `dim` operation index an operand.
ClosedPublic

Authored by frgossen on Jun 10 2020, 4:55 AM.

Details

Summary

Allow for dynamic indices in the dim operation.
Rather than an attribute, the index is now an operand of type index.
This allows to apply the operation to dynamically ranked tensors.
The correct lowering of dynamic indices remains to be implemented.

Diff Detail

Event Timeline

frgossen created this revision.Jun 10 2020, 4:55 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
herhut accepted this revision.Jun 10 2020, 6:41 AM

LGTM from my side with some nits. Thanks for adding this!

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1379

I had to read this multiple times to understand what the dim operation's type is. Its result type is always index. I think you wanted to say something like The specified type is the type of the ... to note what the type after the : means.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2121

I would prefer return failure() here, as that will provide context using the usual mlir machinery rather than assert and abort.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1271

You can use ConstantIndexOp here and avoid creating the attribute explicitly.

This revision is now accepted and ready to land.Jun 10 2020, 6:41 AM
frgossen updated this revision to Diff 269835.Jun 10 2020, 6:53 AM
frgossen marked 3 inline comments as done.

Apply suggestions

This revision was automatically updated to reflect the committed changes.

@frgossen : the pre-merge build is failing on this revision, please check that it is passing or that you understand the failure before pushing.

rriddle added inline comments.Jun 10 2020, 10:51 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2121

nit: Remove the username from the TODO

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
209

nit: Use llvm::TypeSwitch here instead, should be something like:

TypeSwitch<Operation *, bool>(dimOp.memrefOrTensor().getDefiningOp())
  .Case<ViewOp, SubViewOp, AllocOp>([&](auto op) { return isMemRefSizeValidSymbol(op, i, region); })
  .Default([](Operation *) { return false; });
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1277

nit: Remove trivial braces.

silvas added a subscriber: silvas.Jun 11 2020, 3:59 PM

Can you add documentation for what happens in the case where the dimension index is out of bounds? Are we allowed to assume that doesn't happen?

mehdi_amini added inline comments.Jun 11 2020, 5:42 PM
mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir
319

You missed a few captures for the constant values here.

frgossen marked 4 inline comments as done.Jun 12 2020, 4:05 AM

Comments addressed in https://reviews.llvm.org/D81722