LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 51485 - Unnecesary use of incorrect cast in InstructionCombining is causing assertion failure
Summary: Unnecesary use of incorrect cast in InstructionCombining is causing assertion...
Status: CONFIRMED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: Other Linux
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks: release-13.0.1
  Show dependency tree
 
Reported: 2021-08-15 13:29 PDT by Paul Osmialowski
Modified: 2021-10-11 20:29 PDT (History)
6 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Osmialowski 2021-08-15 13:29:29 PDT
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<GetElementPtrInst> 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<X>(Val) && "cast<Ty>() 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<Instruction>(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;
Comment 1 Sanjay Patel 2021-08-15 15:51:59 PDT
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.
Comment 2 Paul Osmialowski 2021-08-16 00:56:38 PDT
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.
Comment 3 Sanjay Patel 2021-08-16 07:05:46 PDT
(In reply to Paul Osmialowski from comment #2)
> 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.
Comment 4 Sanjay Patel 2021-08-16 07:41:12 PDT
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.
Comment 5 Nikita Popov 2021-08-16 08:32:23 PDT
@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.
Comment 6 Sanjay Patel 2021-08-16 08:45:20 PDT
(In reply to Nikita Popov from comment #5)
> @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.)
Comment 7 Sanjay Patel 2021-08-16 08:56:39 PDT
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?