This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Simplify compare of Phi with constant inputs against a constant
ClosedPublic

Authored by mkazantsev on Jun 4 2020, 5:31 AM.

Details

Summary

We can simplify

icmp <pred> phi(C1, C2, ...), C

with

phi(icmp(C1, C), icmp(C2, C), ...)

provided that all comparison of constants are constants themselves.

Diff Detail

Event Timeline

mkazantsev created this revision.Jun 4 2020, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 5:31 AM
lebedev.ri added a subscriber: lebedev.ri.
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
1457

I don't think you need one-use check - you produce a single instruction,
and root icmp from which you started matching always goes away (is replaced by new phi)

1458

Why ConstantInt? What would be wrong with vector of integers? What about undef? Constant exprs?

1467

I'd think you simply want ConstantExpr::getCompare()

mkazantsev marked an inline comment as done.Jun 4 2020, 8:42 PM
mkazantsev added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
1457

Yeah, I wasn't sure if it's ok to change icmp for a phi, but it should be (phis are free if RA makes good job).

mkazantsev marked an inline comment as done.Jun 4 2020, 9:47 PM
mkazantsev added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
1458

Undefs are handled in other place (I will add tests showing that).
Vectors of integers and floats should be fine, I'll add some tests on this.
Constant exprs -- not sure what you mean, but they should become constants at some point, shouldn't they?

mkazantsev marked 3 inline comments as done.Jun 4 2020, 10:32 PM

Addressed comments:

  • Removed single use requirement;
  • Added more tests;
  • Added support for vectors.
mkazantsev edited the summary of this revision. (Show Details)Jun 4 2020, 10:36 PM

Updated other tests.

lebedev.ri accepted this revision.Jun 5 2020, 12:28 AM

LGTM, thanks.

This revision is now accepted and ready to land.Jun 5 2020, 12:28 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 5 2020, 3:25 AM

This breaks tests check-clang: http://45.33.8.238/linux/19487/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This breaks tests check-clang: http://45.33.8.238/linux/19487/step_7.txt

Please take a look and revert for now if it takes a while to fix.

And as is tradition, an overstepping end-to-end test..

RKSimon added a subscriber: RKSimon.Jun 5 2020, 4:50 AM

This looks like its responsible for the exceptions.m test failures on some bots: http://lab.llvm.org:8011/builders/llvm-avr-linux/builds/2189

/home/buildbot-worker/llvm-avr-linux/llvm-avr-linux/llvm/clang/test/CodeGenObjC/exceptions.m:100:12: error: CHECK: expected string not found in input
 // CHECK: [[DEST1:%.*]] = phi i32 [ 0, {{%.*}} ], [ 3, {{%.*}} ]
           ^
<stdin>:195:37: note: scanning from here
 call void @objc_exception_try_exit(%struct._objc_exception_data* nonnull %exceptiondata.ptr) #5
                                    ^
<stdin>:219:2: note: possible intended match here
 %1 = load i32, i32* %x, align 4, !tbaa !7
 ^
fhahn added a comment.Feb 4 2022, 10:01 AM

It looks like this may introduce a dead-code elimination regression with -Oz: https://github.com/llvm/llvm-project/issues/53318

lebedev.ri added inline comments.Feb 4 2022, 10:05 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
1456–1461

Passing-by comment: i strongly suspect these don't actually want to deal with constant *expressions*,
only immediate constants. I missed that originally i guess.