[PATCH] D101680: Change the context instruction for computeKnownBits in LoadStoreVectorizer pass

[Viacheslav Nikolaev via Phabricator via llvm-commits]

wvoquine added a subscriber: rtereshin.
wvoquine added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:521
     KnownBits Known(BitWidth);
-    computeKnownBits(OpA, Known, DL, 0, &AC, OpA, &DT);
     APInt BitsAllowedToBeSet = Known.Zero.zext(IdxDiff.getBitWidth());
----------------
arsenm wrote:
> arsenm wrote:
> > How do we know A is the point the vectorized instruction will be inserted, and not B?
> e.g. can you add some tests with an assume between the two instructions to vectorize?
By the way, we have discussed some of these points with @rtereshin and @bogner 

Neither of OpA and OpB is a point of the vectorization. They both dominate the point of vectorization, but there's no information here about which one is closer to it.

This change only enables more cases to pass: if OpB is nullptr then it means we have bailed long ago in the function:
( See this part:
  // At this point A could be a function parameter, i.e. not an instruction
  Value *ValA = OpA->getOperand(0);
  OpB = dyn_cast<Instruction>(OpB->getOperand(0));
  if (!OpB || ValA->getType() != OpB->getType())
    return false;
)

I just allow it not to bail if OpA appears to be nullptr.

There's no assumption as to which one goes at the point or closer to the point of vectorization.

Also without llvm.assume I couldn't differentiate the two contexts in a test.

And I added a test with the llvm.assume - ld_v2i8_add_different_contexts.

Now, sometimes OpA might be with a better context for the calculation of known bits. In such cases we might fail to vectorize.

If llvm.assume's are inconsistent in the two points then this ought to be an UB.

For an improvement in the future we might need to use the actual vectorization point as the context.


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

https://reviews.llvm.org/D101680