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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:483
 
+// TODO(tpopp): Support witness attributes and then make this ConstantLike.
+// Note: This operation might be replaced with a general op that takes a
----------------
No `TODO` with names please.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:141
 //===----------------------------------------------------------------------===//
-// AssumingOp
+// Assuming
 //===----------------------------------------------------------------------===//
----------------
Why this change?


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:194
+      auto *secondBlock = op.getBody();
+      auto initPosition = rewriter.getInsertionPoint();
+      auto *thirdBlock = rewriter.splitBlock(firstBlock, initPosition);
----------------
Why `initPosition`?


================
Comment at: mlir/test/Dialect/Shape/canonicalize.mlir:91
+// -----
+// Confirm that cstr_eq, assuming_all, assuming, and any can all be folded away
+// when dependent only on constant shapes.
----------------
Some tests where folding does not happen would also be great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80189