TargetLowering.SimplifySetCC - missing icmp eq knownbits combine

[Quuxplusone]

Bugzilla Link	PR41182
Status	NEW
Importance	P enhancement
Reported by	Simon Pilgrim (llvm-dev@redking.me.uk)
Reported on	2019-03-21 08:39:16 -0700
Last modified on	2021-08-09 18:43:21 -0700
Version	trunk
Hardware	PC Windows NT
CC	darcangelo.matthew@gmail.com, david.bolvansky@gmail.com, lebedev.ri@gmail.com, llvm-bugs@lists.llvm.org, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

https://gcc.godbolt.org/z/JahwVe

define i32 @square(i32 %a0)  {
    %1 = and i32 %a0, 127
    %2 = icmp eq i32 %1, -2
    %3 = select i1 %2, i32 42, i32 -17
    ret i32 %3
}

square: # @square
  andl $127, %edi
  cmpl $-2, %edi
  movl $42, %ecx
  movl $-17, %eax
  cmovel %ecx, %eax
  retq

We should be able to constant fold this completely as we know that some of the
known bits will never match:

square: # @square
  movl $-17, %eax
  retq

[Quuxplusone]

Hi,

Looks like no one has taken ownership of this and I'd like to take it on.
Thanks Simon for the detailed info and the expected result.

Thanks,
Matt

[Quuxplusone]

(In reply to Matt D'Arcangelo from comment #1)
> Hi,
>
> Looks like no one has taken ownership of this and I'd like to take it on.
> Thanks Simon for the detailed info and the expected result.

Cheers Matt - something to watch out for is that when I initially attempted
this, I hit issues with branches with constant bool values not collapsing as
expected, so there might be some yak shaving to address.

[Quuxplusone]

(In reply to Simon Pilgrim from comment #2)
> (In reply to Matt D'Arcangelo from comment #1)
> > Hi,
> >
> > Looks like no one has taken ownership of this and I'd like to take it on.
> > Thanks Simon for the detailed info and the expected result.
>
> Cheers Matt - something to watch out for is that when I initially attempted
> this, I hit issues with branches with constant bool values not collapsing as
> expected, so there might be some yak shaving to address.

Thanks for the heads up Simon ( and for adding a new expression to my vocab :) )

Do you have a rough idea on where in the codebase I should look to get started
with this? I started looking around a bit in the debugger but I'm pretty new so
I'm not sure where to begin really without a lot more debugging. Any advice you
have would be greatly appreciated.

Thanks,
Matt

[Quuxplusone]

(In reply to Matt D'Arcangelo from comment #3)
> (In reply to Simon Pilgrim from comment #2)
> > (In reply to Matt D'Arcangelo from comment #1)
> > > Hi,
> > >
> > > Looks like no one has taken ownership of this and I'd like to take it on.
> > > Thanks Simon for the detailed info and the expected result.
> >
> > Cheers Matt - something to watch out for is that when I initially attempted
> > this, I hit issues with branches with constant bool values not collapsing as
> > expected, so there might be some yak shaving to address.
>
> Thanks for the heads up Simon ( and for adding a new expression to my vocab
> :) )
>
> Do you have a rough idea on where in the codebase I should look to get
> started with this? I started looking around a bit in the debugger but I'm
> pretty new so I'm not sure where to begin really without a lot more
> debugging. Any advice you have would be greatly appreciated.

You can find TargetLowering::SimplifySetCC in TargetLowering.cpp, its a very
lengthy function but you should probably try to add these combines near
existing code that is limited to integer comparisons.

You're going to have to make computeKnownBits calls on both operands.

For CondCode::SETEQ and CondCode::SETNE then see if the known one bits from one
intersect with the known zero bits of the other (and vice versa).

For signed/unsigned GT/GE/LT/LE style comparisons you should be able to use the
KnownBits::getMinValue()/getMaxValue() helpers.

What you're probably going to find is that we don't have good tests for all of
this, especially for vectors. Start from the test case above, then add
additional coverage as you go.

[Quuxplusone]

Note that we have redundant but independent known bits analysis functions for
IR and SDAG. It may be useful to reference the IR version(s) of the logic that
you want and their callers.

For example:
https://github.com/llvm/llvm-project/blob/d9e191cb178afd0ab06f4bc932522ddf2e15ac9e/llvm/lib/Analysis/ValueTracking.cpp#L2614

[Quuxplusone]

Thanks Simon and Sanjay, you two have been a huge help.

> > I hit issues with branches with constant bool values not collapsing as
> > expected, so there might be some yak shaving to address.

Simon, do you happen to have some IR that hits this so I can run it against my
changes?

[Quuxplusone]

(In reply to Matt D'Arcangelo from comment #6)
> Thanks Simon and Sanjay, you two have been a huge help.
>
>
> > > I hit issues with branches with constant bool values not collapsing as
> > > expected, so there might be some yak shaving to address.
>
> Simon, do you happen to have some IR that hits this so I can run it against
> my changes?

I can't remember the exact tests but there were a number of regressions when I
ran "ninja check-llvm-codegen-x86" - I'm not sure if I tested any other targets
in the few experiments I ran.

[Quuxplusone]

(In reply to Matt D'Arcangelo from comment #6)
> Simon, do you happen to have some IR that hits this so I can run it against
> my changes?

If you have any concerns, you can put your patch up on phabricator for an early
review.

[Quuxplusone]

(In reply to Simon Pilgrim from comment #8)
> (In reply to Matt D'Arcangelo from comment #6)
> > Simon, do you happen to have some IR that hits this so I can run it against
> > my changes?
>
> If you have any concerns, you can put your patch up on phabricator for an
> early review.

I have a few more regressions that I want to handle first, but that's a good
idea - I'll post an early review this weekend or early next week.