-
Notifications
You must be signed in to change notification settings - Fork 11.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
Unnecesary use of incorrect cast in InstructionCombining is causing assertion failure #50827
Comments
If the crash is is instcombine, we should be able to isolate it to a very small example. Try using "-emit-llvm -mllvm -print-before-all" on your build. You should get the IR just before we hit the assert as the final part of the debug spew. |
I need some advice on how to do that in LTO mode where LLVM is invoked by ld linker as a plugin. Also how "-emit-llvm" fits to that picture? Bear in mind that currently LTO compiler needs hour to reach this assertion. When printing IR at each pass it can take days and produce tons of output, mostly irrelevant. |
I'm not familiar with LTO compile/flags, so I can't help much. I suppose the -emit-llvm is unnecessary if you can get "-mllvm -print-before-all" to work.
Yikes - that sounds bad! Plan B: I git blamed my way to the original commit for this transform: And based on the (unfortunately, not minimal) test cases there, I can imagine what is going wrong. I'm working on a reduction now. |
Can you confirm that this patch works on your build? (hopefully logically equivalent to your suggested fix) https://reviews.llvm.org/rGde285eacb011 I'm making this a candidate for the 13.0 release since it's a small change and avoids a crash. |
@spatel Any reason you did not go with the originally suggested fix? Constexpr GEP does support inbounds, it just needs to be set during construction rather than afterwards. |
Oops, I missed that. I remembered similar code patterns for propagating FMF and no-wrap, so I went with the dyn_cast. Should I revert and commit the better fix or apply the better fix on top? (We'll see the extra 'inbounds' in the regression test either way.) |
Also, is there another bug here? If the 2nd GEP is not 'inbounds', can we propagate 'inbounds' from the 1st GEP alone even though we are re-arranging the operands? |
mentioned in issue #51489 |
The deadline for requesting fixes for the release has passed. This bug is being removed from the LLVM 13.0.1 release milestone. If you have a fix or think this bug is important enough to block the release, please explain why in a comment and add the bug back to the LLVM 13.0.1 release milestone. |
Extended Description
Although this is the same assertion as in #46596, it is not caused by the same problem.
The affected code in llvm/lib/Transforms/InstCombine/InstructionCombining.cpp does a cast to GetElementPtrInst in order to have an
ability to call setIsInBounds() method. Unfortunately, the Value*
pointer returned by the CreateGEP() method does not pass the
isa test checked by the assertion in the cast method. When using the compiler built with assertions enabled, it causes the following
assertion failing (while running LTO compilation mode):
ld: llvm/include/llvm/Support/Casting.h:276: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::GetElementPtrInst; Y = llvm::Value; typename llvm::cast_retty<X, Y*>::ret_type = llvm::GetElementPtrInst*]: Assertion `isa(Val) && "cast() argument of incompatible type!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0. Running pass 'Function Pass Manager' on module 'ld-temp.o'.
Note that this issue is not easy to reproduce and might have stayed
like that for a very long time. The only reason it has revealed itself
was due to our recent works on LTO compilation mode during which we
have tried the compiler with the assertions enabled. The problem
manifested itself eventually when we tried to build a bulky benchmark which is significant in size.
Turns out that the cast which results in the assertion failure can be easily avoided as such:
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2198,9 +2198,9 @@ Instruction *InstCombinerImpl::visitGetElementPtrInst(GetElementPtrInst &GEP) {
// -- have to recreate %src & %gep
// put NewSrc at same location as %src
Builder.SetInsertPoint(cast(PtrOp));
The text was updated successfully, but these errors were encountered: