Currently, we consider any instruction that might read from memory to be unsafe for phi-of-ops.
This patch refines that by walking the clobbering memDefs until we either hit a block that strictly dominates the phi block (safe) or we hit a clobbering memPhi (unsafe). This is analogous to how "regular " SSA values are handled.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
We don't need to check that the memPhi is in the same block to deem it unsafe.
A clobbering memPhi that doesn't strictly dominate the original access implies that there are different versions of memory reaching the load and that using the loaded value between iterations isn't safe.
Comment Actions
LGTM :)
| llvm/lib/Transforms/Scalar/NewGVN.cpp | ||
|---|---|---|
| 2649 | 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){ 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 } | |
| llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll | ||
|---|---|---|
| 80–81 | clobber? | |
Comment Actions
Thanks for the review!
| llvm/lib/Transforms/Scalar/NewGVN.cpp | ||
|---|---|---|
| 2649 | 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. 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()? | |
| llvm/lib/Transforms/Scalar/NewGVN.cpp | ||
|---|---|---|
| 2649 | 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. | |
| llvm/lib/Transforms/Scalar/NewGVN.cpp | ||
|---|---|---|
| 2649 | Will do thanks! | |
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:
loop.header:
bb1:
bb2:
loop.latch:
exit:
}