[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:160
+    OffsetInBits = Offset.getZExtValue() * 8;
+    Alignment = commonAlignment(Alignment, Offset.getZExtValue());
+  }
----------------
spatel wrote:
> lebedev.ri wrote:
> > So we had `SrcPtr`, and split it into a base pointer `SrcPtr'`, and an offset `Offset`.
> > But i think it's `SrcPtr = SrcPtr' + Offset`,
> > so shouldn't the offset be negative here,
> > because the alignment is known for `SrcPtr`, not `SrcPtr'`?
> Good point!
> Intuitively, I couldn't see how it would make a difference if we negated the offset, so I looked at the implementation of MinAlign():
> https://github.com/llvm/llvm-project/blob/4c8276cdc120c24410dcd62a9986f04e7327fc2f/llvm/include/llvm/Support/MathExtras.h#L673
> 
> The optimized IR for that is:
> 
> ```
> define i64 @MinAlign(i64 %A, i64 %B) local_unnamed_addr #0 {
> entry:
>   %or = or i64 %B, %A
>   %add = sub i64 0, %or
>   %and = and i64 %or, %add
>   ret i64 %and
> }
> 
> ```
> 
> Double-check to make sure I didn't mess this up:
> https://alive2.llvm.org/ce/z/wRSjPD
> 
> So it's logically equivalent either way? At the least, I'll add a code comment.
STGM


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

https://reviews.llvm.org/D93229