Page MenuHomePhabricator

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

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



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!


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.


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


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

nit: Remove the username from the TODO


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; });

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

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