[PATCH] D78592: [mlir][SCCP] Add support for propagating across symbol based calls

Mon Apr 27 03:10:52 PDT 2020

bondhugula accepted this revision.
bondhugula added inline comments.
This revision is now accepted and ready to land.

================
Comment at: mlir/lib/Transforms/SCCP.cpp:774
+  // If this terminator is not "return-like", conservatively mark all of the
+  // call-site results as overdefined.
+  auto callableResultLattices = latticeIt->second.getResultLatticeValues();
----------------
rriddle wrote:
> bondhugula wrote:
> > But this terminator might not be the only terminator in the region of the callable op. Why mark call site results overdefined? You'd just propagate to successors the same way as for non-callable op region terminators that aren't return-like. ? You do have a test case ("complex_inner_if") where you have a cond_br in the callable. I was expecting that to not work as a result of this.
> This branch is only taken when there are no CFG successors. This is the same behavior as the region case, i.e. if we see an "exiting" terminator that isn't return-like we assume all exit values are overdefined.
I now see that far above at line 714 - the conditional over the call itself. Locally, this would be misleading including the method name visitCallableTerminatorOperation since these are for term ops that have 0 successors. Could you either update the doc comment or have a comment here to remind the reader? Adding ZeroSucc to the method name will be too long. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78592