This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a pass to remove all shape.cstr_ and assuming_ ops.
ClosedPublic

Authored by tpopp on Jun 10 2020, 5:45 AM.

Details

Summary

This is to provide a utility to remove unsupported constraints or for
pipelines that happen to receive these but cannot lower them due to not
supporting assertions.

Diff Detail

Event Timeline

tpopp created this revision.Jun 10 2020, 5:45 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
herhut added inline comments.Jun 10 2020, 6:27 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
179

Nit: Missing .

mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
22

Could the ones that just erase the op be a template? Also, you can only do this if there are no more users, right? An alternative would be to replace the constraint operations by a true witness and then normal canonicalization should do the rest.

silvas requested changes to this revision.Jun 10 2020, 10:01 AM

What is the invariant established by this pass that other passes can rely on? It seems that this pass is needed for correctness in many cases, not just optimization. So it needs to have a well-specified contract for later passes to rely on.

With that said, I think this pass probably needs to signalPassFailure() in the cases it cannot establish that invariant.

This revision now requires changes to proceed.Jun 10 2020, 10:01 AM
rriddle requested changes to this revision.Jun 10 2020, 10:57 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
541

Why a static method? Also, can you change the name to reflect that it is being inline into the parent? inlineRegionIntoParent?

mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
15

This is not MLIR style, please fix.

Should be using namespace mlir.

80

These should be mlir::

mlir/test/Dialect/Shape/remove-shape-constraints.mlir
21

nit: Remove the extra newline.

tpopp updated this revision to Diff 270791.Jun 15 2020, 10:04 AM
tpopp marked 8 inline comments as done.

Match MLIR style.

I haven't finished handling Sean's comment, but wanted to share my thoughts and pieces so far.

Regarding Sean's comment, I think the guarantee of this pass should be that a success indicates all cstr_ and assuming_ ops are gone.

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

Changed to be non-static. I wasn't sure if it would be safe to delete the op also with a method on the object.

mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
22

Random errors started occurring when I tried templating this, and I'd rather avoid digging through templates right now.

Related to Sean's point. I think this pass should only succeed if all cstr_ or assuming_ ops are deleted (and have no users) and otherwise be a failure. This is really just to remove the information, as otherwise the user would have lowerings.

I tried to make this contained and remove them fully as I thought it's nicer to see if a pass works rather than relying on needing canonicalization after as a correctness mechanism.

rriddle added inline comments.Jun 15 2020, 10:26 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
193

Oops, missed this. Yeah, can you make this static again?

herhut accepted this revision.Jun 15 2020, 11:38 AM
herhut added inline comments.
mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
22

I am not fighting for one way or another of implementing this. However, if the pass would remove all constraint operations then it also has well defined semantics (removes all constraints). The assuming just would be optimized as it always is and relying on canonicalization to clean up is a common pattern.

tpopp updated this revision to Diff 271370.Jun 17 2020, 7:38 AM
tpopp marked 2 inline comments as done.

Change constraint remove to replace cstr_ ops with passing witnesses.

tpopp added inline comments.Jun 17 2020, 7:40 AM
mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
22

I changed my mind on this and changed it to Stephan's way because it will allow more flexibility in which programs this works on in case people start to implement more complicated ideas with Witnesses such as returning them from blocks or using them in their own specialized ops.

tpopp marked an inline comment as done.Jun 17 2020, 7:40 AM
silvas requested changes to this revision.Jun 17 2020, 10:54 AM
silvas added inline comments.
mlir/lib/Dialect/Shape/Transforms/RemoveShapeConstraints.cpp
22

+1, I like that approach.

mlir/test/Dialect/Shape/remove-shape-constraints.mlir
4

can we add a simple test for each op that we remove?

This revision now requires changes to proceed.Jun 17 2020, 10:54 AM
herhut accepted this revision.Jun 18 2020, 1:30 AM

Nice!

mlir/test/Dialect/Shape/remove-shape-constraints.mlir
4

Isn't this what this test does? If we do not remove all, the test will fail?

tpopp updated this revision to Diff 271651.Jun 18 2020, 4:00 AM

Add some tests for individual ops.

tpopp marked 3 inline comments as done.Jun 18 2020, 4:02 AM

I'm going to land this, as I handled Sean's comment, so there doesn't seem to be anything contentious left.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 18 2020, 4:52 AM
This revision was automatically updated to reflect the committed changes.