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

[Manuel Brito via Phabricator via llvm-commits]

ManuelJBrito added inline comments.


================
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:
> ManuelJBrito wrote:
> > 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()?
> Can you please update the comment before you commit the patch?
> 
> It is easy to support the example that I gave you by just refining the code here. We can just read the operands of the memory phi and check if they are constants. The problem is that phi-of-ops optimization will fail because findPHIOfOpsLeader() will fail to find a leader for the new phi node. This happens because findPHIOfOpsLeader() does not have the support for this test case. Anyway, I do not think that this implementation should be part of this patch because it is a special case. My understanding is that this patch aims to be more generic.
Will do thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156055