[PATCH] D93229: [VectorCombine] allow peeking through GEPs when creating a vector load

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:147-149
+    APInt Offset(OffsetBitWidth, 0);
+    SrcPtr = SrcPtr->stripAndAccumulateInBoundsConstantOffsets(DL, Offset);
+    if (!isSafeToLoadUnconditionally(SrcPtr, MinVecTy, Alignment, DL, Load,
----------------
spatel wrote:
> lebedev.ri wrote:
> > I strongly suspect that you need to recalculate the `Alignment` here,
> > because i don't think the `Offset`-less pointer is guaranteed to still be `Alignment`-aligned.
> That's an interesting question. I think what we're doing here (even without this patch) is comparing the alignment of the original scalar load with the alignment of the pointer of a new vector load. But that's not very meaningful is it? For example, if we are loading an i16 with `align 2` then does it matter whether the original pointer is at least `align 2` for a load of v8i16?
> 
> I haven't looked at alignment requirements much. If there's another related transform that we can use as a template, let me know.
I think we don't need to ever ask `isSafeToLoadUnconditionally()` about alignment
(i.e. just pass `0`/`1`, whatever is more permissive),
because we already know the alignment of the load.

We simply need to recalculate the alignment after chopping off the offset
(see `commonAlignment(Align A, uint64_t Offset)`)


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

https://reviews.llvm.org/D93229