-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GVN] Invalidate ICF cache when clearing the instructions #68145
Conversation
This fixes llvm#48805 /home/user/llvm-project/llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:93: void llvm::InstructionPrecedenceTracking::validate(const llvm::BasicBlock *) const: Assertion `It->second == &Insn && "Cached first special instruction is wrong!"' failed.
@llvm/pr-subscribers-llvm-transforms ChangesThis fixes #48805 /home/user/llvm-project/llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:93: void llvm::InstructionPrecedenceTracking::validate(const llvm::BasicBlock *) const: Assertion `It->second == &Insn && "Cached first special instruction is wrong!"' failed. Full diff: https://github.com/llvm/llvm-project/pull/68145.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index bc54846ccf0ad2d..f7a905c2e13c4d4 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2799,6 +2799,7 @@ bool GVNPass::processBlock(BasicBlock *BB) {
salvageDebugInfo(*I);
removeInstruction(I);
}
+ ICF->clear();
InstrsToErase.clear();
if (AtStart)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearing ICF as a whole is not the correct way to fix this. It needs to be incrementally invalidated, as other parts of GVN already do. The first step would be to provide a test case for the assertion failure.
We hit this on AIX while building python with -flto. Here's a reduced testcase:
|
Thanks, @w2yehia, Can you also share how you got this test case for future purposes, I mean how do you run llvm-reduce for lto issue? |
Somewhat reduced test case for declare void @_PyMem_RawFree() nounwind willreturn
define i64 @test(ptr %p) {
entry:
%a = alloca [2 x ptr], align 8
%a2 = getelementptr ptr, ptr %a, i64 1
call void null(ptr %a)
br i1 false, label %if, label %exit
if:
%p0 = load ptr, ptr %a, align 8
%p1 = load ptr, ptr %a2, align 8
br label %exit
exit:
store ptr @_PyMem_RawFree, ptr %p
%p2 = load ptr, ptr %a, align 8
%pgocount.i = load i64, ptr %p2, align 8
%fn = load ptr, ptr %p
tail call void %fn()
%res = load i64, ptr %a2, align 8
ret i64 %res
} |
Fixed in 46aac94. We need to remove users from ICF before performing RAUW. |
Thanks, @nikic for the fix. How do you get the bc/ll file for llvm-reduce i.e. which flag is used when building python? |
I used -save-temps to dump intermediate IR, picked the |
Thanks for the reply, I did set the variable for -save-temps export CC=/home/shivam/llvm/new/llvm-project/build/bin/clang CXX=/home/shivam/llvm/new/llvm-project/build/bin/clang++ CFLAGS="-save-temps" CXXFLAGS="-save-temps" LDFLAGS="-save-temps" And then run It crashes and I checked the generated .bc file There is no *.0.2.internalize.bc, only these
And none of them failed with -passes=lto running with opt. Is this not right? |
on AIX, we use libLTO.so (LTOCodegenerator) and it has the
Try a hello world testcase first, before applying it to the python link. |
@nikic thanks for fixing; I couldn't find out if this was reviewed. If it wasn't, maybe we should? |
Thanks. I get this working by using |
This fixes #48805
/home/user/llvm-project/llvm/lib/Analysis/InstructionPrecedenceTracking.cpp:93: void llvm::InstructionPrecedenceTracking::validate(const llvm::BasicBlock *) const: Assertion `It->second == &Insn && "Cached first special instruction is wrong!"' failed.