This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Shape] Make dimension an operand of `get_extent`
ClosedPublic

Authored by frgossen on Jun 5 2020, 3:41 AM.

Details

Summary

The operation get_extent now accepts the dimension as an operand and is no
longer limited to constant dimensions.
A helper function facilitates the common constant use case.

Depends On D81162

Diff Detail

Event Timeline

frgossen created this revision.Jun 5 2020, 3:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
pifon2a accepted this revision.Jun 5 2020, 5:14 AM
pifon2a added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
191

these "DeclareOp..." trait will disappear after rebase

mlir/lib/Dialect/Shape/IR/Shape.cpp
412

maybe this:

if (auto constSizeOp = dyn_cast<ConstSizeOp>(dim().getDefiningOp()) {
  return constSizeOp.value().getLimitedValue();
}
return llvm::None;
422

static_cast<uint64_t>(elements.getNumElements())

mlir/test/Dialect/Shape/canonicalize.mlir
226

super-nit: empty line after //----

silvas accepted this revision.Jun 5 2020, 10:57 AM
silvas added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
193

Now that we are returning !shape.size, if the shape is an error, or the dim is out of bounds or an error, then the result is an error size. (!shape.size can represent an error)

210

It's slightly confusing that the builder takes int64_t but this uses uint64_t. Can you make them consistent? (for whatever reason, it seems the preference is int64_t).

mlir/lib/Dialect/Shape/IR/Shape.cpp
412

getDefiningOp<ConstSizeOp>() makes it even more succinct. Also you might want to drop the braces for consistency for these small if's.

jpienaar accepted this revision.Jun 5 2020, 12:08 PM
jpienaar added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
210

Preference is for int64_t here yes. I'd prefer we keep that uniformly here.

This revision is now accepted and ready to land.Jun 5 2020, 12:08 PM
frgossen updated this revision to Diff 269143.Jun 8 2020, 2:46 AM
frgossen marked an inline comment as done.

Rebase

frgossen updated this revision to Diff 269144.Jun 8 2020, 3:03 AM
frgossen marked 7 inline comments as done.

Apply suggestions

frgossen marked an inline comment as done.Jun 8 2020, 3:10 AM
frgossen added a subscriber: herhut.
frgossen added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
193

When lowering to std, we will not represent error cases, will we?
At least in that case, the behaviour is undefined.

The way I under stand it is that the error case will be handled by "guarding" assertions (at least at the point at which we lower to std).
In that sense, get_extent has two semantic variants (?).

@herhut

mlir/test/Dialect/Shape/canonicalize.mlir
226

Fixed entire file.

jpienaar added inline comments.Jun 8 2020, 6:23 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
193

That is during the lowering, the op now has explicit behavior in error input case: it propagates the error via the size type. How that semantics is maintained during lowering may change (e.g., using constraint vs assert) but that is on the lowering and not a defining characteristic of the op, the op propagates the error.

herhut accepted this revision.Jun 8 2020, 12:13 PM

Thanks!

mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
193

Yes, the operation should return an error if the shape is an error and also if the dimension is out of bounds. We will have to figure out how we lower this correctly. For the cases we currently care about, the lowering is correct by construction (We iterate over the rank and the shape cannot contain error values.). The latter is due to using constraints but for the former we will need some way to remove bounds checks. To begin with, I would be ok with a lowering that just neglects the bounds checking.

frgossen updated this revision to Diff 269791.Jun 10 2020, 4:44 AM

Rebase, add missing builder

This revision was automatically updated to reflect the committed changes.