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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 3 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:160
+    OffsetInBits = Offset.getZExtValue() * 8;
+    Alignment = commonAlignment(Alignment, Offset.getZExtValue());
+  }
----------------
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.


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

https://reviews.llvm.org/D93229