[PATCH] D80306: [mlir] Canonicalization of shape.assuming_all

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td:1
+include "mlir/Dialect/Shape/IR/ShapeOps.td"
+
----------------
Missing file header?


================
Comment at: mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td:4
+// Constraints
+def AllInputsTrueWitnesses : Constraint<CPred<[{
+        llvm::all_of($0.getOwner()->getOperands(), [](mlir::Value op) {
----------------
Could we indent by 2 instead?

```
def AllInputsTrueWitnesses : Constraint<CPred<[
  llvm::all_of($0.getOwner()->getOperands(), [](mlir::Value op) {
    return op.getDefiningOp<mlir::shape::TrueWitnessOp>();
  })}]>>;
```


================
Comment at: mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td:12
+def ConstantAssumingAll : Pat<(Shape_AssumingAllOp:$op $input),
+          (Shape_TrueWitnessOp),
+          [(AllInputsTrueWitnesses $op)] >;
----------------
Could we just indent these by 2 instead? [yes we need a formatter ... and many other variants as below]

```
def ConstantAssumingAll : Pat<
  (Shape_AssumingAllOp:$op $input), (Shape_TrueWitnessOp),
  [(AllInputsTrueWitnesses $op)]>;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80306