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

[Konstantina Mitropoulou via Phabricator via llvm-commits]

kmitropoulou 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) &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156055