This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Shape] Add `shape.rank` operation
ClosedPublic

Authored by frgossen on Jun 17 2020, 10:31 AM.

Details

Summary

Add shape.rank operation to the shape dialect.

Depends On D82022

Diff Detail

Event Timeline

frgossen created this revision.Jun 17 2020, 10:31 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
silvas requested changes to this revision.Jun 17 2020, 10:43 AM
silvas added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
172

Can we call this "rank", which is the terminology usually used for this? Otherwise, it's quite confusing especially since we have !shape.size which is a totally different thing.

181

can you also add a constant folder?

This revision now requires changes to proceed.Jun 17 2020, 10:43 AM
frgossen marked 2 inline comments as done.Jun 17 2020, 10:52 AM
frgossen added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
172

I see that the name can be confusing.
@pifon2a and I discussed the naming and we though that "rank" would be very confusing as the rank of the argument itself (the shape) is always 1.
What do you think about get_size as a name?

181

Will do that ofc.
Thought that should go in a separate CL.

frgossen updated this revision to Diff 271605.Jun 18 2020, 1:34 AM

Update name

frgossen retitled this revision from [MLIR][Shape] Add `shape.size` operation to [MLIR][Shape] Add `shape.get_size` operation.Jun 18 2020, 1:54 AM
frgossen edited the summary of this revision. (Show Details)
frgossen marked 2 inline comments as done.Jun 18 2020, 1:56 AM
frgossen added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
181
frgossen marked an inline comment as done.Jun 18 2020, 2:53 AM
frgossen added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
181
silvas requested changes to this revision.Jun 18 2020, 9:40 AM
silvas added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
172

I would agree with you if the argument was an extent tensor. But the argument is a !shape.shape. "rank" is unambiguous when applied to something of !shape.shape type.

"size" is super ambiguous: Just based on the name it sounds like it could be any of these ops:

  • shape.num_elements
  • shape.get_rank
  • shape.get_extent
  • shape.const_size

Just as we have been very consistent to use "extent" to mean the size of a dimension for clarity, we should really use the term "rank" here as that has no other possible meaning in this context.

This revision now requires changes to proceed.Jun 18 2020, 9:40 AM
frgossen updated this revision to Diff 272042.Jun 19 2020, 6:30 AM
frgossen marked 2 inline comments as done.

Rename operation

frgossen retitled this revision from [MLIR][Shape] Add `shape.get_size` operation to [MLIR][Shape] Add `shape.rank` operation.Jun 19 2020, 6:31 AM
frgossen edited the summary of this revision. (Show Details)
pifon2a accepted this revision.Jun 19 2020, 9:28 AM
silvas accepted this revision.Jun 19 2020, 12:17 PM
silvas added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
175

nit: just to be precise and self-contained, add "The rank is the number of extents in a shape"

frgossen updated this revision to Diff 272346.Jun 22 2020, 1:31 AM

Address nit

frgossen marked an inline comment as done.Jun 22 2020, 1:32 AM
jpienaar accepted this revision.Jun 23 2020, 2:11 PM
This revision is now accepted and ready to land.Jun 23 2020, 2:11 PM
This revision was automatically updated to reflect the committed changes.