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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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.
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. |
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. |
mlir/lib/Dialect/Shape/IR/Shape.cpp | ||
---|---|---|
193 | Oops, missed this. Yeah, can you make this static again? |
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. |
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. |
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? |
I'm going to land this, as I handled Sean's comment, so there doesn't seem to be anything contentious left.
Why a static method? Also, can you change the name to reflect that it is being inline into the parent? inlineRegionIntoParent?