This is an archive of the discontinued LLVM Phabricator instance.

[shape] Basic constant folding.
ClosedPublic

Authored by silvas on Apr 23 2020, 1:25 PM.

Details

Reviewers
jpienaar
Summary
  • Implement a first constant fold for shape.shape_of (more ops coming in subsequent patches)
  • Implement the right builder interfaces for ShapeType and other types
  • Splits shape.constant into shape.const_size and shape.const_shape which plays better with dyn_cast and building vs one polymorphic op.

Also, fix the RUN line in ops.mlir to properly verify round-tripping.

Depends On D78752 (future patches in the constant-folding patchset will)

Diff Detail

Event Timeline

silvas created this revision.Apr 23 2020, 1:25 PM
Herald added a project: Restricted Project. · View Herald Transcript
silvas updated this revision to Diff 259692.Apr 23 2020, 1:33 PM

Fix include path

silvas updated this revision to Diff 259698.Apr 23 2020, 1:48 PM

use Builder::getType

silvas updated this revision to Diff 259715.Apr 23 2020, 2:43 PM

Fix typo.

mehdi_amini added inline comments.Apr 23 2020, 9:42 PM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
162–163

This needs an update.

163

I don't think we should repeat the summary in the description?

167–168

This example needs an update.

183

It'd be nice to provide the long description like the other above (these will show up on the website)

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

Should handle SizeType here as well?

115

Having a builder and using getI64IntegerAttr(int64_t value); seems like it'd help here

jpienaar added inline comments.Apr 24 2020, 6:18 AM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
157–161

Let's keep these sorted

166

This hasn't been resolved and so should still be left todo

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

Let's keep the ops sorted alphabetical

95

This op doesn't operate only on ShapedType so use dyn_cast instead.

115

Seems wasteful to construct an arrayattr and unique it in context just to print, an interleaveWithComma call should suffice.

silvas updated this revision to Diff 259985.Apr 24 2020, 2:00 PM
silvas marked 9 inline comments as done.

Address comments.

silvas updated this revision to Diff 259988.Apr 24 2020, 2:03 PM
silvas marked 2 inline comments as done.

update

jpienaar accepted this revision.Apr 24 2020, 2:54 PM
jpienaar marked 2 inline comments as done.

Thanks!

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

I'd like to see tests showing each of these being printed (well OK with invalid one not being shown), but it wouldn't be possible without custom parser, so OK to start with the current version.

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

This should use ? for unknown and regular integer for everything else. Adding support for this would mean the IR can't roundtrip, but please add a TODO for follow up

112

Unknown's should be parsed as ? in the descriptions so we would need some additional logic here. E.g., -1 shouldn't be used in the textual form. So a more custom parsing would be required, could you add a TODO here too

119

This seems equally long to

if (auto attr = extent.dyn_cast<IntegerAttr>())
  ints.push_back(attr.getInt());
else
  return failure();

Don't know which one reads easier [this one feels like variable usage scope is more local and makes the two cases clearer but thats just a feelign and not a trong one]

This revision is now accepted and ready to land.Apr 24 2020, 2:54 PM
jpienaar added inline comments.Apr 24 2020, 2:57 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
162

What happens here if we have a error shape fed into ShapeOfOp?

silvas marked 6 inline comments as done.Apr 24 2020, 3:38 PM
silvas added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
98

See other comment.

112

I don't understand the use case for shape constants of partially unknown shape. Maybe we can discuss on discourse?

162

We check that the tensor operand has static shape. So it can't be an error.

silvas marked 3 inline comments as done.Apr 24 2020, 3:39 PM