[PATCH] D156055: [NewGVN][PHIOFOPS] Relax conditions when checking safety of memory accesses

[Manuel Brito via Phabricator via llvm-commits]

ManuelJBrito added a comment.

In D156055#4541541 <https://reviews.llvm.org/D156055#4541541>, @kmitropoulou wrote:

> LGTM :)

Thanks for the review!



================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:2649
+        // Clobbering MemoryPhi - unsafe.
+        // Note : Only checking memory phis allows us to skip constant stores
+        if (isa<MemoryPhi>(MemAccess) &&
----------------
kmitropoulou wrote:
> Can you please give us more details?
> 
> In the following example, the load depends on a memory phi.  But, the check in line 2651 rejects phi-of-ops optimization. This can be optimized If we do some changes in findPHIOfOpsLeader().
> 
> define i32 @mytest(ptr %p, ptr %q, i1 %cond1, i32 %TC){
> entry:
>   br label %loop.header
> 
> loop.header:
>   %phi = phi i32 [0, %entry], [%ind, %loop.latch]
>   br i1 %cond1, label %bb1, label %bb2
> 
> bb1:
>   store i32 100, ptr %p
>   br label %loop.latch
> 
> bb2:
>   store i32 10, ptr %p
>   br label %loop.latch
> 
> loop.latch:
>    %phi2 = phi i32 [1, %bb1], [0, %bb2]
>    %ld = load i32, ptr %p
>    %mul = mul i32 %ld, %phi2
>    %ind = add i32 %phi, 1
>    %cond2 = icmp ule i32 %ind, %TC
>    br i1 %cond2, label %loop.header, label %exit
> 
> exit:
>   ret i32 %mul
> }
> 
> 
This comment should really say redundant stores instead of constant, my bad. It allows us to catch the test case in storeoverstore.ll. Although ideally we shouldn't need such a hack. NewGVN should be able to remove the loads and stores and then do the phi-of-ops.
If we run NewGVN back to back it does just that.[https://godbolt.org/z/Wef4WW5ec ]

Regarding your example ideally I think this should be done by phi-translating the MemoryPhis  - we currently lose a few optimizations because we don't.

What changes do you propose in findPHIOfOpsLeader()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156055