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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1031
+  if (MatchShiftRecurrence(O, StartV, StepV)) {
+    computeKnownBits(StartV, DemandedElts, Known2, Depth + 1, Q);
+    switch(Opcode) {
----------------
reames wrote:
> 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.
Yeah, I think you're right that keeping the context instruction would be correct in this case.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1411
+          computeKnownBits(R, DemandedElts, Known2, Depth + 1, RecQ);
+          switch(Opcode) {
+          case Instruction::Shl:
----------------
nit: `switch (`


================
Comment at: llvm/test/Analysis/ValueTracking/shift-recurrence-knownbits.ll:64
 
 ; negative case for when start is unknown
 define i64 @test_ashr_unknown(i64 %start) {
----------------
I'd suggest another negative case where the wrong operand of the shift is used (lshr i64 C, %phi style).

And another positive test where the order of the phi operands is swapped.


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

https://reviews.llvm.org/D96440