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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula requested changes to this revision.
bondhugula added a comment.
This revision now requires changes to proceed.

Some more comments. Again, I've been looking at things a bit locally. Will take a global look in the  next round. Are your test cases still missing scenarios like:

1. paths propagating different constants meeting (you have one with back edges (the last one) but none with the straightforward if/else style).
2. a test case where you have say a multi operand op (say addi) that has one operand coming directly from a constant op and the other via a block argument whose both predecessors send in the same constant.



================
Comment at: mlir/lib/Transforms/SCCP.cpp:323
+  if (op->getNumResults() == 0 || op->getNumRegions() != 0)
+    return markAllOverdefined(op->getResults());
+
----------------
But getResults() is empty!


================
Comment at: mlir/lib/Transforms/SCCP.cpp:353
+
+  // Try to fold the result of this operation to a constant. If folding fails or
+  // was an in-place fold, mark the results as overdefined.
----------------
Nit: Try to fold -> Simulate folding .... This is the part that's contradictory in the face of FoldUtils::tryToFold and the comment above "Don't try to fold ..."


================
Comment at: mlir/lib/Transforms/SCCP.cpp:358
+  if (failed(op->fold(operandConstants, foldResults)))
+    return markAllOverdefined(op->getResults());
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397