[PATCH] D66232: [InstCombine] Try to reuse constant from select in leading comparison

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/unrecognized_three-way-comparison.ll:92-106
 define i32 @compare_against_two(i32 %x) {
 ; CHECK-LABEL: @compare_against_two(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp sgt i32 [[X:%.*]], 2
-; CHECK-NEXT:    br i1 [[TMP0]], label [[CALLFOO:%.*]], label [[EXIT:%.*]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32 [[X:%.*]], 2
+; CHECK-NEXT:    [[CMP2_INV:%.*]] = icmp sgt i32 [[X]], 1
+; CHECK-NEXT:    [[SELECT1:%.*]] = select i1 [[CMP2_INV]], i32 1, i32 -1
+; CHECK-NEXT:    [[SELECT2:%.*]] = select i1 [[CMP1]], i32 0, i32 [[SELECT1]]
----------------
xbolva00 wrote:
> lebedev.ri wrote:
> > xbolva00 wrote:
> > > lebedev.ri wrote:
> > > > This is clearly a regression.
> > > > I suppose it means this fold happens before some other fold that used to happen,
> > > > and that fold does not know how to deal with the new pattern.
> > > > I'd prefer to look into this afterwards.
> > > No status update here? Or did you find what is broken?
> > > 
> > > Patch looks fine I think, but this worries me -   we may realize later that the root cause of regression is possibly nontrivial thing which could lead us to rever this one. (and wasted time).
> > Added some tests in rL369667.
> > Whatever handles this is "simply" ignorant about commutativity.
> Ok, thanks for info.
> 
> (Not a patch blocker)
The problems appear to begin in `InstCombiner::foldICmpSelectConstant()`,
which uses `InstCombiner::matchThreeWayIntCompare()`:
```
bool InstCombiner::matchThreeWayIntCompare(SelectInst *SI, Value *&LHS,
                                           Value *&RHS, ConstantInt *&Less,
                                           ConstantInt *&Equal,
                                           ConstantInt *&Greater) {
  // TODO: Generalize this to work with other comparison idioms or ensure
  // they get canonicalized into this form.

  // select i1 (a == b), i32 Equal, i32 (select i1 (a < b), i32 Less, i32
  // Greater), where Equal, Less and Greater are placeholders for any three
  // constants.
```
Not unsurprisingly, it does not handle any other pattern :/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66232/new/

https://reviews.llvm.org/D66232