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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Thanks for the review!



================
Comment at: mlir/lib/Transforms/SCCP.cpp:156
+
+  /// Mark the edge between 'from' and 'to' as executable.
+  void markEdgeExecutable(Block *from, Block *to);
----------------
bondhugula wrote:
> Nit: do either backticks and regular single quotes work for parameters in doc comments?
I'm not sure as we don't generate doxygen ATM for me to look at. The codebase is fairly inconsistent to either, but changed all within this file to consistently use ''. We should figure that out though, and standardize to using one.


================
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.
----------------
bondhugula wrote:
> 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
> ```
This is a bit of a tricky situation. We don't actually want to fold here, this is essentially just simulated execution with constant parameters. The constants we have here aren't guaranteed to be those at runtime, so it's better to avoid the extra overhead of OperationFolder as we don't want to generated anything at this point.


================
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
----------------
bondhugula wrote:
> Could this reuse tryToFold?
Same comment above. We don't want to generate any constant operations here, we just want to know what the output would be. As stated above, the constant values are the current values from the lattice not the current IR. So even if we did use it, it wouldn't give us the result we want.


================
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>()]);
+    }
----------------
bondhugula wrote:
> Trivial braces.
Thanks for the catch, they weren't trivial at one point but I forgot to cleanup.


================
Comment at: mlir/test/Transforms/sccp.mlir:125-126
+^bb5(%result: i32):
+  // CHECK: ^bb5(%{{.*}}: i32):
+  // CHECK: return %[[CST]] : i32
+
----------------
bondhugula wrote:
> 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)
Added a few extra checks for making sure various things get folded. ATM we just insert constants. I've debated back and forth about adding in the CFG canonicalizations here, but given that `canonicalize` can already take care of those I decided to keep the initial versions focused on the propagation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397