[PATCH] D80189: [mlir] Add canonicalization for Cstr and Assuming Shape Ops.

[Sean Silva via Phabricator via llvm-commits]

silvas added inline comments.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:106
+// Removes AnyOp with constant shape input.
+// TODO(tpopp): This case can be replaced with folding.
+// TODO(tpopp): Another pattern should be implemented for shapes that can be
----------------
Use a fold.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:186
+// Removes AssumingOp with a passing condition and inlines the region.
+struct AssumingWithTrue : public OpRewritePattern<AssumingOp> {
+  using OpRewritePattern<AssumingOp>::OpRewritePattern;
----------------
We might not want this pattern always. For example, if we have a pass that does code motion on islands, this canonicalization can prematurely remove the island which will make that pass unable to operate properly.  Let's hold off on adding it until we have a compelling use case.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:325
+    // Don't try to compare equality when the shapes are not constant.
+    auto lhsShape = op.getOperand(0).getDefiningOp<ConstShapeOp>().shape();
+    auto rhsShape = op.getOperand(1).getDefiningOp<ConstShapeOp>().shape();
----------------
This will segfault if getDefiningOp returns null.


================
Comment at: mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td:19
+def ConstantAssumingAll : Pat<(Shape_AssumingAllOp:$op $input),
+          (Shape_TrueWitnessOp),
+          [(AllInputsTrueWitnesses $op)] >;
----------------
I don't think that TableGen is buying us much here. I prefer loose C++ patterns since they are easier to debug and understand.


================
Comment at: mlir/test/Dialect/Shape/canonicalize.mlir:119
+  %0 = shape.cstr_broadcastable(%cs0, %cs1)
+  %3 = "shape.assuming"(%0) ({
+    shape.assuming_yield %cs1 : !shape.shape
----------------
Can you make this test have less op surface area? Maybe use an unknown op to capture the witness (or return the witness) instead of having the shape.assuming?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80189/new/

https://reviews.llvm.org/D80189