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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D78397#1990284 <https://reviews.llvm.org/D78397#1990284>, @bondhugula wrote:

> 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.


Added a test case for 1, 2 doesn't seem to cover any new code paths. There is already a test for folding an addi that has an input from a loop iv.



================
Comment at: mlir/lib/Transforms/SCCP.cpp:323
+  if (op->getNumResults() == 0 || op->getNumRegions() != 0)
+    return markAllOverdefined(op->getResults());
+
----------------
bondhugula wrote:
> But getResults() is empty!
getResults isn't necessarily empty if the operations has regions. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397