This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
ClosedPublic

Authored by xbolva00 on Jul 6 2019, 7:12 AM.

Details

Summary

(select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y)) -> ashr (X, Y))
(select (icmp slt x, 1), ashr (X, Y), lshr (X, Y)) -> ashr (X, Y))

Fixes PR41173

Alive proof by @lebedev.ri (thanks)
Name: PR41173

%cmp = icmp slt i32 %x, 1
%shr = lshr i32 %x, %y
%shr1 = ashr i32 %x, %y
%retval.0 = select i1 %cmp, i32 %shr1, i32 %shr
=>
%retval.0 = ashr i32 %x, %y

Optimization: PR41173
Done: 1
Optimization is correct!

Diff Detail

Event Timeline

xbolva00 created this revision.Jul 6 2019, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2019, 7:12 AM
xbolva00 edited the summary of this revision. (Show Details)Jul 6 2019, 7:12 AM
xbolva00 updated this revision to Diff 208272.Jul 6 2019, 7:15 AM

Updated tests

spatel added a comment.Jul 6 2019, 7:48 AM

If this transform always returns an existing value, it can go in InstSimplify. Please pre-commit the baseline tests (in the InstSimplify directory and change the RUN line if I got that right).

What is one of the shifts has the "exact" flag set and the other doesn't?

lebedev.ri requested changes to this revision.Jul 8 2019, 11:28 PM

Thanks for working on this!

If this transform always returns an existing value, it can go in InstSimplify. Please pre-commit the baseline tests (in the InstSimplify directory and change the RUN line if I got that right).

I agree.

What is one of the shifts has the "exact" flag set and the other doesn't?

We'd still drop one of two shifts, and a select, so this is really good.
But indeed. We can do this in InstSimplify only if both of the shifts either had or had no exact:
https://rise4fun.com/Alive/t4Z
https://rise4fun.com/Alive/P6a
https://rise4fun.com/Alive/gfP9

This revision now requires changes to proceed.Jul 8 2019, 11:28 PM
xbolva00 updated this revision to Diff 208701.Jul 9 2019, 8:34 AM
xbolva00 retitled this revision from [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y)) to [InstSimplify] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y)).

Moved to InstSimplify.
Precommited tests.
Handled "exact" variants.

Are we really allowed to change the exact flag from InstSimplify?

lebedev.ri requested changes to this revision.Jul 9 2019, 8:45 AM

Thank you for working on this!

lib/Analysis/InstructionSimplify.cpp
82–85 ↗(On Diff #208701)

There was some utility function to check for sign-bit tests, i think, i can't find it..

90–92 ↗(On Diff #208701)

This modifies existing instruction.
I really don't think this is legal for instsimplify.

The mismatch case should stay in instcombine.

This revision now requires changes to proceed.Jul 9 2019, 8:45 AM
spatel added a comment.Jul 9 2019, 8:46 AM

Are we really allowed to change the exact flag from InstSimplify?

No - we're not supposed to morph existing values here.

I wasn't thinking about the 'exact' case when I made the earlier comment, so we should move this back to instcombine. Sorry about that.

xbolva00 marked an inline comment as done.Jul 9 2019, 9:47 AM
xbolva00 added inline comments.
lib/Analysis/InstructionSimplify.cpp
82–85 ↗(On Diff #208701)

Thanks for hint!

I found it, decomposeBitTestICmp.

lebedev.ri marked an inline comment as done.Jul 9 2019, 10:03 AM
lebedev.ri added inline comments.
lib/Analysis/InstructionSimplify.cpp
82–85 ↗(On Diff #208701)

Yep, i think that's the one.

xbolva00 updated this revision to Diff 208754.Jul 9 2019, 11:25 AM
xbolva00 marked an inline comment as done.

Moved back to InstCombine.

xbolva00 marked 2 inline comments as done.Jul 9 2019, 11:25 AM
xbolva00 added inline comments.
lib/Analysis/InstructionSimplify.cpp
82–85 ↗(On Diff #208701)

Sadly, we cannot use it here. E.g.: slt x, 1

xbolva00 retitled this revision from [InstSimplify] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y)) to [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y)).Jul 9 2019, 11:26 AM

Moved back to InstCombine.

I think we are going in circles here.
Part of this fold belongs to instsimplify: (either both are exact, or both non-exact, or ashr non-exact)
https://rise4fun.com/Alive/pleX
And whatever remains (only ashr is exact) should be folded to ashr i32 %x, %y in instcombine.
https://rise4fun.com/Alive/MSpl

lib/Transforms/InstCombine/InstCombineSelect.cpp
547–548

Actually, i don't think this is precise: https://rise4fun.com/Alive/Njpt

Part of this fold belongs to instsimplify:

almost same code on two places :/

Part of this fold belongs to instsimplify:

almost same code on two places :/

Ugly, yeah, but powerful instsimplify is nice ^^

lebedev.ri requested changes to this revision.Jul 10 2019, 8:12 AM

(no new comments, just marking as reviewed)

This revision now requires changes to proceed.Jul 10 2019, 8:12 AM
xbolva00 updated this revision to Diff 209178.Jul 11 2019, 4:56 AM

Updated, addressed review notes.

xbolva00 marked an inline comment as done.Jul 11 2019, 4:56 AM
lebedev.ri added inline comments.Jul 11 2019, 5:25 AM
test/Transforms/InstCombine/ashr-lshr.ll
2–3

Precommit please :)

nikic added a subscriber: nikic.Jul 11 2019, 5:34 AM

Ugly, yeah, but powerful instsimplify is nice ^^

I feel pretty strongly that this is the not the right tradeoff to make here. Unless there's some specific evidence that we benefit from having this in instsimplify, reducing code duplication is imho much more valuable than making sure that one out of thousands of ad-hoc folds runs in a few more cases than it otherwise would...

lebedev.ri requested changes to this revision.Jul 11 2019, 5:51 AM

Nice, pretty much there!

lib/Analysis/InstructionSimplify.cpp
73–74 ↗(On Diff #209178)
///   (select (icmp sgt x, C), lshr (X, Y), ashr (X, Y)); iff C s>= -1
///   (select (icmp slt x, C), ashr (X, Y), lshr (X, Y)); iff C s>= 0
77–78 ↗(On Diff #209178)

I think it would be less confusing to reword as

/// iff either `ashr` is not `exact`, or `lshr` is 'exact' too.
86 ↗(On Diff #209178)
/// Canonicalize so that `ashr` is in FalseVal.
92 ↗(On Diff #209178)
/// If either the `ashr` is non-`exact`, or `lshr` is exact too, then we can just return the `ashr` instruction.

i think it may be less confusing to flip the if accordingly.

97–99 ↗(On Diff #209178)

early return?

lib/Transforms/InstCombine/InstCombineSelect.cpp
539

I don't think we want to make any such restrictions.

/// into:
///   ashr (X, Y)
/// It is assumed that InstSimplify has already run.
547–565

Same comments as to instsimplify function - comments, early return

557–558

No need for this check.

559

I'd really recommend to create a new instruction.
It will result in same source line count, but is that much more transparent to the pass,
and is e.g. visible in -debug output.

This revision now requires changes to proceed.Jul 11 2019, 5:51 AM

Ugly, yeah, but powerful instsimplify is nice ^^

I feel pretty strongly that this is the not the right tradeoff to make here. Unless there's some specific evidence that we benefit from having this in instsimplify, reducing code duplication is imho much more valuable than making sure that one out of thousands of ad-hoc folds runs in a few more cases than it otherwise would...

Feel free to bring this up as RFC on llvm-dev.

lebedev.ri added inline comments.Jul 11 2019, 5:56 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
559

Err
+
/// This new ashr must not be exact.

xbolva00 updated this revision to Diff 209201.Jul 11 2019, 6:19 AM
xbolva00 marked 3 inline comments as done.

Fixed forgotten note.

xbolva00 marked 4 inline comments as done.Jul 11 2019, 6:20 AM
lebedev.ri added inline comments.Jul 11 2019, 6:23 AM
lib/Analysis/InstructionSimplify.cpp
92–93 ↗(On Diff #209178)

Hmm, also, as i've recently seen, cast<BinaryOperator> is going to break for constant expressions.
Let's just do the most obvious thing instead:

if (match(TrueVal, m_LShr(m_Value(X), m_Value(Y))) &&
    match(FalseVal, m_AShr(m_Specific(X), m_Specific(Y))) &&
    match(CmpLHS, m_Specific(X)) && 
    (!match(FalseVal, m_Exact()) || match(TrueVal, m_Exact())))
lebedev.ri added inline comments.Jul 11 2019, 6:26 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
562

There is no legality reason for this check.
We won't (should not) get here in any other case other than when we need to produce the exact-less ashr.

xbolva00 planned changes to this revision.Jul 11 2019, 6:27 AM
xbolva00 marked an inline comment as done.Jul 11 2019, 6:32 AM
xbolva00 added inline comments.
lib/Analysis/InstructionSimplify.cpp
92–93 ↗(On Diff #209178)

m_Exact() not exists?

xbolva00 marked an inline comment as done.Jul 11 2019, 6:36 AM
xbolva00 added inline comments.
lib/Analysis/InstructionSimplify.cpp
92–93 ↗(On Diff #209178)

m_Exact(m_BinOp()).. okay

xbolva00 updated this revision to Diff 209204.Jul 11 2019, 6:37 AM

Fixed notes.

xbolva00 planned changes to this revision.Jul 11 2019, 6:41 AM
xbolva00 updated this revision to Diff 209206.Jul 11 2019, 6:44 AM

Updated. Hopefully almost there.

xbolva00 marked 5 inline comments as done and an inline comment as not done.Jul 11 2019, 6:44 AM

It'd be really great to have at just least one positive non-splat-vector test for instsimplify and instcombine for each predicate (i.e. 4 more tests?)

lib/Analysis/InstructionSimplify.cpp
82–84 ↗(On Diff #209204)

This is not going to work with non-splat vector constants, although it is trivial to make it,
i'd recommend to do this instead:

unsigned Bitwidth = CmpRHS->getType()->getScalarSizeInBits();
if ((Pred != ICmpInst::ICMP_SGT ||
     !match(CmpRHS,
            m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, -1)))) &&
    (Pred != ICmpInst::ICMP_SLT ||
     !match(CmpRHS,
            m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, 0))))) {

(m_SpecificInt_ICMP() is really fresh)

92–93 ↗(On Diff #209178)

Ugh, right, sorry.
Though maybe m_Instruction(), not m_BinOp(); this really asks for m_Anything() matcher..

lib/Transforms/InstCombine/InstCombineSelect.cpp
548–553

Same remark about constant

561

Likewise, i think it's sufficient to cast to Instruction..

564

nice!

xbolva00 marked an inline comment as done.Jul 11 2019, 8:50 AM
xbolva00 added inline comments.
lib/Analysis/InstructionSimplify.cpp
92–93 ↗(On Diff #209178)

m_Exact(m_Instruction()) - does not work too.

a) use m_Exact(m_Instruction(DummyInst))
b) m_Exact(m_BinOp())

?

xbolva00 updated this revision to Diff 209236.Jul 11 2019, 8:51 AM
xbolva00 marked 4 inline comments as done.

Fixed some review notes.

xbolva00 updated this revision to Diff 209239.Jul 11 2019, 9:00 AM

Introduced m_Instruction().

lebedev.ri added a comment.EditedJul 11 2019, 9:47 AM

Ugly, yeah, but powerful instsimplify is nice ^^

I feel pretty strongly that this is the not the right tradeoff to make here.

Unless there's some specific evidence that we benefit from having this in instsimplify,
reducing code duplication is imho much more valuable than making sure that
one out of thousands of ad-hoc folds runs in a few more cases than it otherwise would...

Feel free to bring this up as RFC on llvm-dev.

To elaborate, i don't disagree that this is truly a death of a thousand cuts,
*all* these folds are ad-hoc. Some will do wonders for some code patterns,
some will be useless drag for most of the code.

While theoretically we could teach the peephole pass all about
every possible pattern, it is infeasible in reality.
(at least until instcombine-SMT is here, will that be a glorious day!).

So what is feasible is teaching it about patterns that *were* observed
in real code. This is what i'm trying to do with my patches,
and i'd guess this may be the case with this patch, @xbolva00?

As to the question in the context of this patch, whether to only have the InstCombine part,
or duplicate it in InstSimplify thus not prohibiting other passes from being smarter,
is this a question of code duplication, or redundant matching?
The former can be workarounded with some refactoring to have a single function,
former is still the same death of a thousand cuts and can't be reasonably addressed
within existing framework.

lib/Analysis/InstructionSimplify.cpp
92–93 ↗(On Diff #209178)

Blargh, sorry for leading this in circles.
Then let's go sideways and just do

if (match(TrueVal, m_LShr(m_Value(X), m_Value(Y))) &&
    match(FalseVal, m_AShr(m_Specific(X), m_Specific(Y))) &&
    match(CmpLHS, m_Specific(X)) &&
    (!cast<Instruction>(FalseVal)->isExact() ||
     cast<Instruction>(TrueVal)->isExact()))

as this appears to least mad solution *here*.

(yeah, I saw this missed optimization on the rust github issue tracker)

Ugly, yeah, but powerful instsimplify is nice ^^

I feel pretty strongly that this is the not the right tradeoff to make here.

Unless there's some specific evidence that we benefit from having this in instsimplify,
reducing code duplication is imho much more valuable than making sure that
one out of thousands of ad-hoc folds runs in a few more cases than it otherwise would...

Feel free to bring this up as RFC on llvm-dev.

To elaborate, i don't disagree that this is truly a death of a thousand cuts,
*all* these folds are ad-hoc. Some will do wonders for some code patterns,
some will be useless drag for most of the code.

While theoretically we could teach the peep-hole pass all about
every possible pattern, it is infeasible in reality.
(at least until instcombine-SMT is here, will that be a glorious day!).

So what is feasible is teaching it about patterns that *were* observed
in real code. This is what i'm trying to do with my patches,
and i'd guess this may be the case with this patch, @xbolva00?

As to the question in the context of this patch, whether to only have the InstCombine part,
or duplicate it in InstSimplify thus not prohibiting other passes from being smarter,
is this a question of code duplication, or redundant matching?
The former can be workarounded with some refactoring to have a single function,
former is still the same death of a thousand cuts and can't be reasonably addressed
within existing framework.

I think my comment came across more negative than intended. I think this is a useful patch (e.g. I remember seeing this pattern reported in https://github.com/rust-lang/rust/issues/58083). What I disagree with is the "premature compiler optimization" that is sometimes happening. The original motivation for this patch is very likely fully addressed by just having the InstCombine portion of this patch. But when new combines are implemented, it often happens that their scope is extended beyond what we know to be necessary, into more speculative territory.

Often these extensions are beneficial, for example because it is better to handle a more general pattern than some specific cases, or because a more general pattern naturally falls out of the matching mechanisms we use (typical example: splat vectors). But in some cases these extensions seem to happen more "because we can" than out of a reasonable expectation of utility.

In this specific case, yes, technically we could get an additional improvement by having both the instsimplify and instcombine folds, but it comes at the cost of both duplicate code and duplicate matching overhead. In such cases, a value judgement has to be made that may differ case-by-case. Is the additional complexity and compile-time cost worth the additional optimization opportunities? In this specific case, I don't think so, as it is rather unlikely the inststimplify fold will trigger in a context where the instcombine fold would not. But I understand that this is something two reasonable people may disagree on ;)

So to answer your question, it's about both code duplication and redundant matching. And while the death by a thousand cuts may be unavoidable, we should still try to not hasten it along unduly...

(I defer to your judgement though. If you think having the instsimplify variant is worthwhile, I definitely don't want to block this.)

So to answer your question, it's about both code duplication and redundant matching. And while the death by a thousand cuts may be unavoidable, we should still try to not hasten it along unduly...

I agree. Let's not add extra code for the 'exact' case if we don't need to. Ie, if we can do all of these transforms in instcombine alone by dropping the 'exact' flag, do that. Put another way: unless there's evidence that dropping the exact flag is going to cause a missed optimization, go with the 'less code is better' solution. We have precedence in instcombine/instsimplify for that trade-off.

xbolva00 updated this revision to Diff 209301.Jul 11 2019, 12:32 PM
lebedev.ri accepted this revision.Jul 11 2019, 12:33 PM

Sorry for so may triparounds :(
Please re-re-precommit the tests first.
I guess this will work, with 2 nits addressed :)

So to answer your question, it's about both code duplication and redundant matching. And while the death by a thousand cuts may be unavoidable, we should still try to not hasten it along unduly...

I agree. Let's not add extra code for the 'exact' case if we don't need to. Ie, if we can do all of these transforms in instcombine alone by dropping the 'exact' flag, do that. Put another way: unless there's evidence that dropping the exact flag is going to cause a missed optimization, go with the 'less code is better' solution. We have precedence in instcombine/instsimplify for that trade-off.

I think you were thinking of something else here, as this does not represent the problem at hand.
We fully can do this fold within instcombine, preserving exact when possible.
By having instsimplify fold we, well, enrich instsimplify utility.

lib/Transforms/InstCombine/InstCombineSelect.cpp
565–567

This is way more covoluted than it should be:

bool IsExact = Ashr->isExact() && cast<Instruction>(TrueVal)->isExact();
568

You already matched X and Y.

569

[optional] Do we really care about the name? I'd think we should take one from select then, not that it is accessible here.

This revision is now accepted and ready to land.Jul 11 2019, 12:33 PM
xbolva00 updated this revision to Diff 209303.Jul 11 2019, 12:41 PM
xbolva00 marked 3 inline comments as done.

Fixes

It should be fine now.

Thank you all for review notes!

This revision was automatically updated to reflect the committed changes.

compiler now crashes on this file from CTMark https://reviews.llvm.org/F9549420

FAILED: CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o 
/usr/local/google/home/vitalybuka/src/llvm.git/out/ts_zn_true_Os_32/tools/timeit --summary CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o.time /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/bin/clang -DNDEBUG  -m32     -Os   -w -Werror=date-time -fno-strict-aliasing -DCLOCK_NO_TIMING -MD -MT CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o -MF CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o.d -o CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o   -c /usr/local/google/home/vitalybuka/src/llvm.git/test-suite/CTMark/SPASS/list.c
clang: ../../llvm-project/llvm/include/llvm/ADT/APInt.h:279: llvm::APInt::APInt(unsigned int, uint64_t, bool): Assertion `BitWidth && "bitwidth too small"' failed.
Stack dump:
0.	Program arguments: /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/bin/clang -cc1 -triple i386-unknown-linux-gnu -emit-obj -disable-free -main-file-name list.c -mrelocation-model static -mthread-model posix -relaxed-aliasing -fmath-errno -masm-verbose -mconstructor-aliases -fuse-init-array -target-cpu pentium4 -dwarf-column-info -debugger-tuning=gdb -coverage-notes-file /usr/local/google/home/vitalybuka/src/llvm.git/out/ts_zn_true_Os_32/CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.gcno -resource-dir /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/lib/clang/9.0.0 -dependency-file CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o.d -sys-header-deps -MT CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o -D NDEBUG -D CLOCK_NO_TIMING -internal-isystem /usr/local/include -internal-isystem /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/lib/clang/9.0.0/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -Os -Werror=date-time -w -fdebug-compilation-dir /usr/local/google/home/vitalybuka/src/llvm.git/out/ts_zn_true_Os_32 -ferror-limit 19 -fmessage-length 0 -fobjc-runtime=gcc -fdiagnostics-show-option -vectorize-loops -vectorize-slp -faddrsig -o CTMark/SPASS/CMakeFiles/SPASS.dir/list.c.o -x c /usr/local/google/home/vitalybuka/src/llvm.git/test-suite/CTMark/SPASS/list.c 
1.	<eof> parser at end of file
2.	Per-module optimization passes
3.	Running pass 'CallGraph Pass Manager' on module '/usr/local/google/home/vitalybuka/src/llvm.git/test-suite/CTMark/SPASS/list.c'.
4.	Running pass 'Combine redundant instructions' on function '@list_CompareElementDistribution'
 #0 0x000000000296d71d llvm::sys::PrintStackTrace(llvm::raw_ostream&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Support/Unix/Signals.inc:494:13
 #1 0x000000000296d71d PrintStackTraceSignalHandler(void*) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Support/Unix/Signals.inc:554:0
 #2 0x000000000296b3fe llvm::sys::RunSignalHandlers() /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #3 0x000000000296d8d8 SignalHandler(int) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1
 #4 0x00007f342ae543a0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0)
 #5 0x00007f3429f7ecfb raise (/lib/x86_64-linux-gnu/libc.so.6+0x36cfb)
 #6 0x00007f3429f698ad abort (/lib/x86_64-linux-gnu/libc.so.6+0x218ad)
 #7 0x00007f3429f6977f (/lib/x86_64-linux-gnu/libc.so.6+0x2177f)
 #8 0x00007f3429f77542 (/lib/x86_64-linux-gnu/libc.so.6+0x2f542)
 #9 0x0000000002b401e1 llvm::InstCombiner::foldSelectInstWithICmp(llvm::SelectInst&, llvm::ICmpInst*) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:0:0
#10 0x0000000002b41b87 llvm::InstCombiner::visitSelectInst(llvm::SelectInst&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1963:31
#11 0x0000000002ab13d2 llvm::InstCombiner::run() /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3289:22
#12 0x0000000002ab2d6a combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, bool, llvm::LoopInfo*) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3523:13
#13 0x0000000002ab4338 llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3597:10
#14 0x00000000027a642f llvm::FPPassManager::runOnFunction(llvm::Function&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1648:27
#15 0x00000000024bc7e7 (anonymous namespace)::CGPassManager::RunPassOnSCC(llvm::Pass*, llvm::CallGraphSCC&, llvm::CallGraph&, bool&, bool&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:176:25
#16 0x00000000024bc7e7 (anonymous namespace)::CGPassManager::RunAllPassesOnSCC(llvm::CallGraphSCC&, llvm::CallGraph&, bool&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:441:0
#17 0x00000000024bc7e7 (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:497:0
#18 0x00000000027a6e56 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1752:27
#19 0x00000000027a6e56 llvm::legacy::PassManagerImpl::run(llvm::Module&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1865:0
#20 0x0000000001b06e4b (anonymous namespace)::EmitAssemblyHelper::EmitAssembly(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/CodeGen/BackendUtil.cpp:902:3
#21 0x0000000001b06e4b clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1549:0
#22 0x0000000002106e5d std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >::~unique_ptr() /usr/lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/unique_ptr.h:273:6
#23 0x0000000002106e5d clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:303:0
#24 0x0000000003417a75 __gnu_cxx::__normal_iterator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >*, std::vector<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >, std::allocator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> > > > >::__normal_iterator(std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >* const&) /usr/lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/stl_iterator.h:781:20
#25 0x0000000003417a75 std::vector<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >, std::allocator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> > > >::begin() /usr/lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/stl_vector.h:699:0
#26 0x0000000003417a75 void clang::finalize<std::vector<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >, std::allocator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> > > > >(std::vector<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >, std::allocator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> > > >&, clang::Sema const&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/include/clang/Sema/TemplateInstCallback.h:54:0
#27 0x0000000003417a75 clang::ParseAST(clang::Sema&, bool, bool) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/Parse/ParseAST.cpp:178:0
#28 0x0000000002065250 clang::FrontendAction::Execute() /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/Frontend/FrontendAction.cpp:938:10
#29 0x0000000001fcf871 llvm::Error::getPtr() const /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/include/llvm/Support/Error.h:273:42
#30 0x0000000001fcf871 llvm::Error::operator bool() /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/llvm/include/llvm/Support/Error.h:236:0
#31 0x0000000001fcf871 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/Frontend/CompilerInstance.cpp:944:0
#32 0x000000000210205f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:291:25
#33 0x0000000001ac0975 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/tools/driver/cc1_main.cpp:249:15
#34 0x0000000001ace43b ExecuteCC1Tool(llvm::ArrayRef<char const*>, llvm::StringRef) /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/tools/driver/driver.cpp:309:12
#35 0x0000000001ace43b main /usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/../../llvm-project/clang/tools/driver/driver.cpp:381:0
#36 0x00007f3429f6b52b __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2352b)
#37 0x0000000001ac002a _start (/usr/local/google/home/vitalybuka/src/llvm.git/out/zn_true/bin/clang+0x1ac002a)
lebedev.ri added inline comments.Jul 12 2019, 1:31 PM
llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp
547 ↗(On Diff #209458)

So this returns 0 i guess?
That is confusing..

xbolva00 marked an inline comment as done.Jul 12 2019, 1:41 PM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp
547 ↗(On Diff #209458)

Yeah, weird.

@spatel ?

getScalarSizeInBits probably returns 0 for pointers. Only DataLayout knows pointer size.

if (Bitwidth == 0) {

  IC->dump();
}

%cmp.i = icmp ne %struct.LIST_HELP* %List.addr.0, null

getScalarSizeInBits probably returns 0 for pointers. Only DataLayout knows pointer size.

Thanks

if (Bitwidth == 0) {

  IC->dump();
}

%cmp.i = icmp ne %struct.LIST_HELP* %List.addr.0, null

Yep, its a pointer. Just disable the transform if !CmpRHs->getType()->isIntOrIntVectorTy()

if (Bitwidth == 0) {

  IC->dump();
}

%cmp.i = icmp ne %struct.LIST_HELP* %List.addr.0, null

Yep, its a pointer. Just disable the transform if !CmpRHs->getType()->isIntOrIntVectorTy()

Thanks! Commited fast fix. And thanks @vitalybuka for reporting this issue.

rL365959