[PATCH] D96440: [knownbits] Preserve known bits for small shift recurrences

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1031
+  if (MatchShiftRecurrence(O, StartV, StepV)) {
+    computeKnownBits(StartV, DemandedElts, Known2, Depth + 1, Q);
+    switch(Opcode) {
----------------
nikic wrote:
> The context instruction here needs to be adjusted to the terminator of the block for the incoming value StartV. Context can't be preserved when looking through phis.
In this particular case, I believe the current usage is actually correct.  The general problem with phis is that you can end up traversing a value which is not control dependent on the conditions controlling the original context.  In this particular case, we know that the phi dominates the binop which dominates the original context (or at least it had better), and thus we should be okay to preserve the initial context.

Having said all that, I hadn't intended to be fancy here and don't care about the potential quality gain from preserving the context.  I'm going to switch to using the phi one as a defensive programming measure anyways.  It's much more obviously correct.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1466
+        // recurrence itself to refine the result.
+        RefineRecurrence(LU, DemandedElts, Known, Known2, Depth, Q);
+
----------------
nikic wrote:
> The surrounding code here is already handling other types of recurrences of the same form -- combining it with RefineRecurrence is rather odd, because it does it's own recurrence matching that is partially redundant with the code here. I think these should be integrated, or at least factored differently. The scaffolding here should work for the shift case as well (restricted to LL == I, as for Sub below), just with different KnownBits logic being applied.
I acknowledge this ends up being slightly odd for this caller.  I want to preserve the matching logic structure anyways.  Specifically, immediately after this lands, I have several other use cases for recognizing shift recurrences.  

If you really want, I can invert and generalize the existing code here and defer the addition of the new matcher to a following patch.  I wouldn't bother myself, but don't care strongly enough to really argue against it either.  


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

https://reviews.llvm.org/D96440