[llvm] Fix to generalize the canSkipClobberingStore (PR #174137)

[via llvm-commits]

Bhuvan1527 wrote:

> Hi, there is a related miscompilation test case:
> 
> ```llvm
> define i32 @test_gep_alias(ptr %p, ptr %p2, i32 %arg) {
> entry:
>   store i32 0, ptr %p
>   store i32 %arg, ptr %p2
>   %v = load i32, ptr %p
>   ret i32 %v
> }
> ```
> 
> After this patch, it's transformed into:
> 
> ```llvm
> define i32 @test_gep_alias(ptr %p, ptr %p2, i32 %arg) {
> entry:
>   store i32 0, ptr %p, align 4
>   store i32 %arg, ptr %p2, align 4
>   ret i32 0
> }
> ```
> 
> The original transformation is:
> 
> ```llvm
> define i32 @test_gep_alias(ptr %p, ptr %p2, i32 %arg) {
> entry:
>   store i32 0, ptr %p, align 4
>   store i32 %arg, ptr %p2, align 4
>   %v = load i32, ptr %p, align 4
>   ret i32 %v
> }
> ```
> 
> Alive2 proof: https://alive2.llvm.org/ce/z/h5yBWm.
> 
> > Note: This is a review assisted with a self-built AI agent. The reproducer was validated manually. Please let me know if anything is wrong.
> 
> The fix is fundamentally unsound because it assumes: If the store’s pointer operand has only one SSA use (`SIPtr->hasOneUse()`), then the underlying memory it points to is not accessed anywhere else.
> 
> This is not valid in LLVM IR:
> 
> * `hasOneUse()` is a property of an SSA value, not of the memory location.
> * Multiple distinct SSA pointer values can refer to the same memory via aliasing.
> 
> So even if `%p2` is only used by one store instruction, it can still alias `%p`, and `%p` is used by the load. Therefore the store to `%p2` can still clobber the load from `%p`, and skipping it is incorrect.
> 
> **Consequence:** The analysis may ignore real clobbers, enabling GVN to forward stale/older values across intervening stores, producing wrong-code.

Thank you for pointing the mistake @cardigan1008 . 

https://github.com/llvm/llvm-project/pull/174137