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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked an inline comment as done.
bondhugula added a comment.

Overall, this code looks really good. I can do a complete review unless Jacques/Mehdi who were originally added as reviewers plan to review it anyway.



================
Comment at: mlir/lib/Transforms/SCCP.cpp:358
+  if (failed(op->fold(operandConstants, foldResults)))
+    return markAllOverdefined(op->getResults());
+
----------------
rriddle wrote:
> bondhugula wrote:
> > Why isn't this conservative? If you mark it overdefined already, it can never be highered again. What if some of its operand lattice values are later lowered from unknown to constant? Are you treating both unknown and overdefined in the same way here for simulated folding purposes?
> It already is conservative, if you look above we only get to this point if all of the operands have been resolved. At this point the operands can only go to overdefined, so there isn't a way that more information can be propagated to this point.
Thanks - that's what I missed - the check above for unknown operands. 


================
Comment at: mlir/lib/Transforms/SCCP.cpp:89
+  /// lattice value changed.
+  bool mergeIn(const LatticeValue &rhs) {
+    // If we are already overdefined, or rhs is unknown, there is nothing to do.
----------------
The standard term for this in the SCCP paper and the literature is `meet` - these are the meet rules. `mergeIn` -> `meet`?


================
Comment at: mlir/lib/Transforms/SCCP.cpp:131
+  /// uses of all values that have been computed to be constant, and erases as
+  /// any newly dead operations.
+  void rewrite(MLIRContext *context, MutableArrayRef<Region> regions);
----------------
Typo: any -> many.


================
Comment at: mlir/lib/Transforms/SCCP.cpp:455
+    const LatticeValue &operandLattice = latticeValues[(*branchOperands)[i]];
+    LatticeValue &argLattice = latticeValues[arg];
+    updatedLattice |= argLattice.mergeIn(operandLattice);
----------------
`argLattice` is actually invariant? Hoist it out of the loop, and use it in the block above where you are marking it overdefined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397