[PATCH] D79717: [MLIR] Add shape.witness type and ops

[Sean Silva via Phabricator via llvm-commits]

silvas accepted this revision.
silvas added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:24
+  let typeDescription = [{
+    A witness is a token to ordering of code using relying on information
+    obtained from passing assertions. Witnesses do not represent any physical
----------------
typo "using relying"


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:25
+    A witness is a token to ordering of code using relying on information
+    obtained from passing assertions. Witnesses do not represent any physical
+    data, so they do not exist at execution time.
----------------
"They are only used as a structural device in the compiler to maintain ordering."


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:32
+    "assuming_" operations will take witnesses as input and represent only
+    information to the compiler, so they do not exist in executing code. Cod
+    that is dependent on "assuming_" operations can assume all cstr operations
----------------
typo Cod


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:397
+
+def Shape_AssumeAllOp : Shape_Op<"assuming_all", []> {
+  let summary = "Return a logical AND of all witnesses.";
----------------
def name inconsistent with op name.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:434
+
+def Shape_ExecAssuming : Shape_Op<"assuming_region",
+                           [SingleBlockImplicitTerminator<"AssumingRegionYield">,
----------------
def name is inconsistent with op name.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:294
+//===----------------------------------------------------------------------===//
+LogicalResult
+AnyOp::inferReturnTypes(MLIRContext *context, Optional<Location> location,
----------------
nit: newline after block comment


================
Comment at: mlir/test/Dialect/Shape/ops.mlir:71
+  %w1 = "shape.cstr_eq"(%0, %1) : (!shape.shape, !shape.shape) -> !shape.witness
+  %2 = "shape.any"(%0, %1)  : (!shape.shape, !shape.shape) -> !shape.shape
+  %w3 = "shape.all"(%w0, %w1) : (!shape.witness, !shape.witness) -> !shape.witness
----------------
Let's move the shape.any into the region?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79717