[PATCH] D79036: [SCCP] Switch to widen at PHIs, stores and call edges.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

(just realized I never submitted the responses to the inline comments)



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:741
       continue;
+    NumActiveIncoming++;
 
----------------
nikic wrote:
> The lazy counting of active values might be problematic: It works fine if phi args become feasible from left to right, but not if it happens from right to left. In that case, upon hitting the newly active argument, we will always set MaxWidenSteps to 2, regardless of how many of the later arguments are already active.
Yes I meant to operate on a fresh lattice value that's latter combined with the existing value. I've reworked the code to do the merging a bit differently.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:746
+        Res.mergeIn(IV, ValueLatticeElement::MergeOptions().setMaxWidenSteps(
+                            NumActiveIncoming + 1));
     if (Res.isOverdefined())
----------------
nikic wrote:
> The main problem with this heuristic is that it does not distinguish which incoming value contributes to the widening. Intuitively, what we want is to allow widening once per operand, but what could end up happening is that we have a phi node with 64 arguments, 63 of them are constant, and the last one is an addrec. In this case, we would end up executing the loop 64 times. Of course, it's something of a degenerate case...
> 
> I don't have an immediate suggestion on how to improve this at the same level of complexity though.
yeah, I don't think there's too much we can do about that without extra tracking. I've updated the code to set the number of range extensions to the max of the number of range extensions and the number of active incoming values. In the case were first merging lots of equal constants, we do not allow multiple extensions for later extending ranges. I think that's a bit better, but there are still cases which are problematic (e.g. if the first value that becomes feasible is an add-rec).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79036