Skip to content
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

Jump to bottom

Unnecesary use of incorrect cast in InstructionCombining is causing assertion failure #50827

Open
llvmbot opened this issue Aug 15, 2021 · 9 comments
Open

Unnecesary use of incorrect cast in InstructionCombining is causing assertion failure #50827

llvmbot opened this issue Aug 15, 2021 · 9 comments
Labels
bugzilla Issues migrated from bugzilla confirmed Verified by a second party

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2021

Bugzilla Link 51485
Version trunk
OS Linux
Blocks #51489
Reporter LLVM Bugzilla Contributor
CC @RKSimon,@nikic,@rotateright,@tstellar

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'.

  1.  Running pass 'Combine redundant instructions'

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));

  •        auto *NewSrc = cast<GetElementPtrInst>(

  •            Builder.CreateGEP(GEPEltType, SO0, GO1, Src->getName()));

  •        NewSrc->setIsInBounds(Src->isInBounds());

  •        auto *NewSrc = Src->isInBounds() ?

  •          Builder.CreateInBoundsGEP(GEPEltType, SO0, GO1, Src->getName()) :

  •          Builder.CreateGEP(GEPEltType, SO0, GO1, Src->getName());
       auto *NewGEP = GetElementPtrInst::Create(GEPEltType, NewSrc, {SO1});
       NewGEP->setIsInBounds(GEP.isInBounds());
       return NewGEP;
@rotateright
Copy link
Contributor

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 16, 2021

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.

@rotateright
Copy link
Contributor

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?

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.

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.

Yikes - that sounds bad!

Plan B: I git blamed my way to the original commit for this transform:
https://reviews.llvm.org/D45287

And based on the (unfortunately, not minimal) test cases there, I can imagine what is going wrong.

I'm working on a reduction now.

@rotateright
Copy link
Contributor

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.

@nikic
Copy link
Contributor

nikic commented Aug 16, 2021

@​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.

@rotateright
Copy link
Contributor

@​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.)

@rotateright
Copy link
Contributor

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?

@tstellar
Copy link
Collaborator

mentioned in issue #51489

@tstellar tstellar mentioned this issue Oct 12, 2021
Closed
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@asl asl added this to the LLVM 13.0.1 release milestone Dec 12, 2021
@tstellar
Copy link
Collaborator

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.

@tstellar tstellar removed this from the LLVM 13.0.1 release milestone Dec 21, 2021
@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla confirmed Verified by a second party
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@asl @tstellar @nikic @rotateright @llvmbot