[InstCombine] Remove Store when its Ptr is removable #79565
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: hanbeom (ParkHanbum) ChangesIf the This patch removes the Full diff: https://github.com/llvm/llvm-project/pull/79565.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
index bb2a77daa60a764..94a0d0cc4acc58a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
@@ -1385,9 +1385,10 @@ Instruction *InstCombinerImpl::visitStoreInst(StoreInst &SI) {
if (Ptr->hasOneUse()) {
if (isa<AllocaInst>(Ptr))
return eraseInstFromFunction(SI);
- if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr)) {
- if (isa<AllocaInst>(GEP->getOperand(0))) {
- if (GEP->getOperand(0)->hasOneUse())
+ if (isa<GetElementPtrInst>(Ptr) || isa<AddrSpaceCastInst>(Ptr)) {
+ Instruction *PtrI = dyn_cast<Instruction>(Ptr);
+ if (isa<AllocaInst>(PtrI->getOperand(0))) {
+ if (PtrI->getOperand(0)->hasOneUse())
return eraseInstFromFunction(SI);
}
}
diff --git a/llvm/test/Transforms/InstCombine/store.ll b/llvm/test/Transforms/InstCombine/store.ll
index 673395464c85aaf..969d766a2d1e2bf 100644
--- a/llvm/test/Transforms/InstCombine/store.ll
+++ b/llvm/test/Transforms/InstCombine/store.ll
@@ -345,6 +345,22 @@ define void @store_to_readonly_noalias(ptr readonly noalias %0) {
ret void
}
+; if ptr which store to is addrspacecast, and its target is alloca. remove it.
+define void @src_store_oneuse_addrspaceast_alloca(ptr align 8 %arg) {
+; CHECK-LABEL: @src_store_oneuse_addrspaceast_alloca(
+; CHECK-NEXT: bb:
+; CHECK-NEXT: store ptr poison, ptr null, align 8
+; CHECK-NEXT: ret void
+;
+bb:
+ %i = alloca ptr, align 8, addrspace(5)
+ %i1 = addrspacecast ptr addrspace(5) %i to ptr
+ store ptr %arg, ptr %i1, align 8
+ %i2 = load ptr, ptr %i1, align 8
+ store ptr %i2, ptr null, align 8
+ ret void
+}
+
!0 = !{!4, !4, i64 0}
!1 = !{!"omnipotent char", !2}
!2 = !{!"Simple C/C++ TBAA"}
|
| if (isa<AllocaInst>(GEP->getOperand(0))) { | ||
| if (GEP->getOperand(0)->hasOneUse()) | ||
| if (isa<GetElementPtrInst>(Ptr) || isa<AddrSpaceCastInst>(Ptr)) { | ||
| Instruction *PtrI = dyn_cast<Instruction>(Ptr); |
There was a problem hiding this comment.
Unchecked dyn_cast, use cast<>
| %i = alloca ptr, align 8, addrspace(5) | ||
| %i1 = addrspacecast ptr addrspace(5) %i to ptr | ||
| store ptr %arg, ptr %i1, align 8 |
There was a problem hiding this comment.
This test case doesn't contain a getelementptr. I don't see how anything could have gone wrong previously? Either way, should have a test with and without GEP
There was a problem hiding this comment.
to summarize, do you mean that a test with GEP is also necessary? but imo, this issue have not related with GEP. I would appreciate it if you could tell me why you are comment about GEP.
There was a problem hiding this comment.
I mean you changed the handling under the dyn_cast. As such I don't see how this could possibly fix anything for the AddrSpaceCastInst case. There's nothing special about AddrSpaceCast specifically. I assume the same would manifest for any transitive pointer user, so I don't understand how this is avoiding the issue
There was a problem hiding this comment.
@arsenm Now I understand!! I think it would be solve to implement a function like visitallocsite in the alloca instruction. thanks!!
ParkHanbum
force-pushed
the
issue_68120
branch
from
1ff87fc
to
62e63cd
Compare
|
I left a comment about the issue explaining how to fix it. I will close this pull request as it seems unnecessary until this issue is resolved. may I? @arsenm |
ParkHanbum
force-pushed
the
issue_68120
branch
from
62e63cd
to
9d76995
Compare
ParkHanbum
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Previously, if the `Store`'s `Ptr` was `Alloca` and satisfy OneUse() it would be removed. However, if `Alloca` was overlapped and referenced by instructions such as `GEP` or `AddrSpaceCast`, it could not be removed and an error occurred as a result. This patch fixes this by having the `Store`'s `Ptr` find out if the target is `Alloca`, and remove the `Store` if possible.
ParkHanbum
force-pushed
the
issue_68120
branch
from
9d76995
to
21b364f
Compare
There was a problem hiding this comment.
This issue is Won't Fix as far as I'm concerned. Reaching a fix-point in all cases is an explicit non-goal -- the verification exists to catch bugs like failing to add things back to the worklist. You should not perform contortions to reach the fixpoint, unless there is phase-ordering proof that doing so would be useful.
If this is the case, I think the current behavior of error-by-default isn't really reasonable. You should be able to run instcombine standalone without triggering errors like this I think this should be fixed, but I feel like it should be nowhere near this complicated to address |
|
I think this is resolvable by just using stripPointerCasts in visitStoreInst when it's looking for the alloca/GEP source |
Our requirement is that fix-point verification is enabled by default when writing instcombine tests, to prevent accidental introduction of worklist management bugs. The way we currently do this is to make Those are basically the constraints here -- happy to hear alternative ways we could have this "verify in tests, don't verify in production" behavior. Possibly the error message just needs to be improved to make it clear that this is not necessary a problem?
This will miscompile cases if there are multiple uses along the stripped chain, I think? |
I think the message definitely should be improved. Downgrade the message to warning?
I started guarding against these, but so far I haven't had much luck avoiding the second iteration in the mixed multiple use cases. |
If the
storetarget is OneUse and the user isAddrSpaceCastInst, proper processing was not performed. Therefore, when processingAllocain the second process, this storage space was deleted and a problem occurred.This patch removes the
storewhen the store target is OneUse and the user isAddrSpaceCastInst.This is a sample IR a crash occurred. crafted by @arsenm
RUN RECORD:
$ bllvm0/bin/clang --version
clang version 18.0.0git (git@github.com:ParkHanbum/llvm-project.git 0633226612a37933ebd41e523bb9383d80b4cf6a)
$ bllvm0/bin/opt --passes=instcombine crash.ll -debug
ANALYZE :
This problem seems to come from incomplete processing of the
addrspacecastinstruction.In the first instcombine process, after the load instruction is removed as follows:
In the processing the first instruction
allocin the second instcombine process,isAllocSiteRemovableinside thevisitAllocSitefunction is called.In this function, the
addrspacecastandstoreinstructions are removed.This is the processing that should have occurred when the
loadinstruction was removed. However, addrspacecast does not do this.