[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Biplob Mishra via Phabricator via llvm-commits]

bipmis added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:508
+      Load1Ptr = BC->getOperand(0);
+  }
+
----------------
nikic wrote:
> This whole code can be replaced with `stripAndAccumulateConstantOffsets()`.
Sure. Will look into this.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AArch64/or-load.ll:203
+; ALL-NEXT:    [[L3:%.*]] = load i8, ptr [[P2]], align 1
+; ALL-NEXT:    store i8 10, ptr [[PSTR:%.*]], align 1
+; ALL-NEXT:    [[L4:%.*]] = load i8, ptr [[P3]], align 1
----------------
nikic wrote:
> Try a variant storing to `%p3` rather than `%pstr` here. I believe your current implementation will incorrectly accept this.
 It does not return an "available value" for a direct clobber. For example a change to the test 
  %l1 = load i8, ptr %p
  %l2 = load i8, ptr %p1
  %l3 = load i8, ptr %p2
  store i8 10, i8* %p3
  %l4 = load i8, ptr %p3

still returns
; LE-NEXT:    [[TMP1:%.*]] = load i16, ptr [[P]], align 1
; LE-NEXT:    [[TMP2:%.*]] = zext i16 [[TMP1]] to i32
; LE-NEXT:    [[L3:%.*]] = load i8, ptr [[P2]], align 1
; LE-NEXT:    store i8 10, ptr [[P3]], align 1
; LE-NEXT:    [[L4:%.*]] = load i8, ptr [[P3]], align 1

Can add more tests if you suggest.


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

https://reviews.llvm.org/D127392