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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
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
----------------
bipmis wrote:
> 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.
Okay, I had to test this patch locally to find a case where it fails. Try this variant:
```
  store i8 0, ptr %p3
  store i8 1, ptr %p
```
We are looking for an available value of `%p`, so we find the `store i8 1, ptr %p` and are happy. But before that, there is a clobber of `%p3` that makes the transform invalid (the load is moved before the store).


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

https://reviews.llvm.org/D127392