This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold ((x?1:4)&(y?1:4))==0 to x^y
AbandonedPublic

Authored by marcauberer on Sep 15 2022, 12:39 AM.

Details

Reviewers
spatel
RKSimon
Summary

Fixes issue #57666: https://github.com/llvm/llvm-project/issues/57666

The optimization applies to all constant values that equal to zero when
applied to an bitwise and instruction.

Alive2: https://alive2.llvm.org/ce/z/RrVEyX
Minimalistic Godbolt example: https://godbolt.org/z/3d6h9z4de

Diff Detail

Event Timeline

marcauberer created this revision.Sep 15 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 12:39 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
marcauberer requested review of this revision.Sep 15 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 12:39 AM

Will execute clang-format later

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6486

@spatel do you know how to check for shared bits between C and D the best. The current solution does not work.
Besides that, by feeding C and D with m_APInt to the match function, the vector test case does not work properly. We might need to fall back to m_Value, do we?

spatel added inline comments.Sep 15 2022, 6:40 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6486

Before making enhancements to generalize the fold, we have to make sure the transform is sound.

The Alive2 proof says that the select values must be non-zero, but that is not checked. We need more tests to verify that the matching is complete.

Once you're confident that the transform is correct for the basic pattern, look at how to make it handle other patterns:

  1. Why are there one-use restrictions? Hint: if we're only creating 1 instruction, then we can never end up with more instructions than we started with.
  2. Almost any fold that checks for "X == 0" alone can be generalized to handle "X != 0".
  3. We don't need to capture all of the constants if we're looking for equivalent values; use m_Specific or m_Deferred for that. See m_ImmConstant() for a more general match than m_APInt().
RKSimon requested changes to this revision.Sep 16 2022, 4:05 AM
RKSimon added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6486

You need to use m_Deferred to match later matches of C/D to their first occurrence.

Also, try to use llvm::haveNoCommonBitsSet/isKnownNonZero to match the general cases

This revision now requires changes to proceed.Sep 16 2022, 4:05 AM

Consider more checks and icmp ne as well

Add negative tests

marcauberer marked 2 inline comments as done.Sep 16 2022, 4:49 PM
marcauberer added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6486

Seems like m_Deferred does work in combination with m_Value, but not with m_ImmConstant or m_Constant. Thus I went back to m_Value ...
I also added the variant for "X != 0" in addition to "X == 0" and extended the baseline tests accordingly.

spatel added inline comments.Sep 19 2022, 7:34 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6480

The formatting is off.

6481–6483

This shouldn't use the commutative matcher (m_c_And); it should be m_And() with m_Specific().
Ie, we match operand 0 of the 'and' as any select; then, we match operand 1 with constraints based on whatever was matched as operand 0.

6485

If we're using the more general ValueTracking API's, then there should be at least one test for each code path that uses non-constants to exercise that more expensive analysis.

This patch is getting bigger than I originally anticipated, so I'd be fine with making that a small follow-up patch, but see if @RKSimon agrees.

6488

xot -> not ?

6490–6492

It shouldn't matter whether the selects have other uses. We're creating 2 new instructions, so if the 'and' has only one use, then and+icmp will be deleted, and the transform doesn't increase instruction count.

Code improvements

marcauberer marked 3 inline comments as done.Sep 19 2022, 11:30 AM

As of tomorrow I am on vacation for about two weeks. I hope we can put this on hold until I am available again ...

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6480

That is on purpose to maintain overwiew ...
Will format the change later on

6481–6483

Yes, indeed. Changed it.

marcauberer marked an inline comment as done.

Rebase and format

marcauberer marked an inline comment as done.Oct 27 2022, 1:13 PM
marcauberer added inline comments.
llvm/test/Transforms/InstCombine/select_and_icmpne.ll
88

That result might be confusing, but as you see on the original test result, the 'ugt' predicate gets transformed to 'ne' and then the transformation can take effect.

@marcauberer reverse ping - what's happening with this patch please?

Apply latest baseline test changes

goldstein.w.n added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
6489

It would reduce alot of duplicate code to handle ne with the eq code and just add the not if Pred == ICMP_NE.

Also probably should add alive2 link for this case too:
https://alive2.llvm.org/ce/z/NhgW3z

RKSimon resigned from this revision.Feb 7 2023, 10:51 AM
spatel resigned from this revision.Feb 11 2023, 5:53 AM

Basic support added with:
rG98855059674c
rGeb6f5f1ada6a

There are TODO comments in the code about extending the matching in a variety of ways.

marcauberer abandoned this revision.May 21 2023, 1:37 PM