[PATCH] D78397: [mlir][Transforms] Add pass to perform sparse conditional constant propagation

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
Comment at: mlir/lib/Transforms/SCCP.cpp:43-44
+
+    /// A value with dynamic values that cannot be represented. This state
+    /// cannot be changed.
+    Overdefined
----------------
Nit: `A value with dynamic values` can be confusing - instead, `A value that cannot statically be determined to be a constant`


================
Comment at: mlir/lib/Transforms/SCCP.cpp:129
+
+  /// Rewrite the given regions using the computing analysis.
+  void rewrite(MLIRContext *context, MutableArrayRef<Region> regions);
----------------
Could you expand this comment?


================
Comment at: mlir/lib/Transforms/SCCP.cpp:156
+
+  /// Mark the edge between 'from' and 'to' as executable.
+  void markEdgeExecutable(Block *from, Block *to);
----------------
Nit: do either backticks and regular single quotes work for parameters in doc comments?


================
Comment at: mlir/lib/Transforms/SCCP.cpp:313
+
+    // Don't try to fold the results as we can't guarantee folds won't be
+    // in-place.
----------------
You can with the last output parameter on tryToFold - is this what you were looking for?

```
bool inPlaceUpdate;
FoldUtils::tryToFold(op, nullptr, nullptr, &inPlaceUpdate);
// operation is folded if inPlaceUpdate is false
```


================
Comment at: mlir/lib/Transforms/SCCP.cpp:344-363
+  // Save the original operands and attributes just in case the operation folds
+  // in-place. The constant passed in may not correspond to the real runtime
+  // value, so in-place updates are not allowed.
+  SmallVector<Value, 8> originalOperands(op->getOperands());
+  NamedAttributeList originalAttrs = op->getAttrList();
+
+  // Try to fold the result of this operation to a constant. If folding fails or
----------------
Could this reuse tryToFold?


================
Comment at: mlir/lib/Transforms/SCCP.cpp:373-377
+    if (Attribute foldAttr = foldResult.dyn_cast<Attribute>()) {
+      mergeIn(op, resultLattice, LatticeValue(foldAttr, opDialect));
+    } else {
+      mergeIn(op, resultLattice, latticeValues[foldResult.get<Value>()]);
+    }
----------------
Trivial braces.


================
Comment at: mlir/test/Transforms/sccp.mlir:100
+
+// CHECK-LABEL: func @simple_loop_inner_control_flow
+func @simple_loop_inner_control_flow(%arg0 : i32) -> i32 {
----------------
Mention here to the effect that this is not just inner control flow but you are testing sensitivity to  non-executable edges in the CFG.


================
Comment at: mlir/test/Transforms/sccp.mlir:121-123
+  %cst_50 = constant 50 : i32
+  br ^bb1(%cst_50 : i32)
+
----------------
You could make this a little stronger/realistic by just doing an IV increment here. 
```
%iv_inc = addi %iv, %cst_1
br ^bb1(%iv_inc)
```
Still the same result. 




================
Comment at: mlir/test/Transforms/sccp.mlir:125-126
+^bb5(%result: i32):
+  // CHECK: ^bb5(%{{.*}}: i32):
+  // CHECK: return %[[CST]] : i32
+
----------------
This looks good, but reading the test case in isolation won't make it clear as to what happens to the non-executable edge/path post SCCP on this test case. Does it stay as is, or becomes a dead/unreachable block, or is deleted? Add additional checks depending on what it is? (I just jumped here without looking at all of the actual code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397