[PATCH] D86364: [ValueTracking] Interpret GEPs as a series of adds multiplied by the related scaling factor

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/constant-fold-gep.ll:47
+; CHECK: store i32 1, i32* getelementptr inbounds ([3 x %struct.X], [3 x %struct.X]* @Y, i64 0, i64 2, i32 1, i64 2), align 4
+  store i32 1, i32* getelementptr ([3 x %struct.X], [3 x %struct.X]* @Y, i64 0, i64 0, i32 0, i64 17), align 4
 ; CHECK: store i32 1, i32* getelementptr inbounds ([3 x %struct.X], [3 x %struct.X]* @Y, i64 1, i64 0, i32 0, i64 0), align 8
----------------
qcolombet wrote:
> Note: The change in the input of the test is because the alignment was not consistent with the base pointer.
> On this store the alignment cannot be 8, since the base pointer is aligned on 16 (17 * 4 + 16 == 17 * 4 + 4 * 4 == 21 * 4 == 10 * 8 + 4 i.e., remainder with 8 cannot be zero). More easily seen looking at the previous load: @Y + 16 is aligned to 8 then we add 4 to make @Y + 17, and we claimed it was aligned to 8.
I agree that the changes were necessary, otherwise the stores should always raise UB due to the alignment mismatch.
It seems that the last three stores below are always raising UB as well - the size of `@Y` is 72 bytes, so the accesses below are out-of-bounds.
Should we treat these in this patch (by removing the three lines or expanding the size of the global variable), or is it okay to just leave them as they were?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86364