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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1299
+
+  auto ConstantsAreElementWiseEqual = [](Constant *Cx, Value *Y) {
+    // Are they fully identical?
----------------
spatel wrote:
> This seems like it would be generally useful as a Constant query function, so it'd be nice to move it over to Constant.h either before or after this commit.
Indeed, will move afterwards.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1320
+  // Does this constant C match any of the `select` values?
+  auto Check = [ConstantsAreElementWiseEqual, SelVal0, SelVal1](Constant *C) {
+    return ConstantsAreElementWiseEqual(C, SelVal0) ||
----------------
spatel wrote:
> 'Check' is a bit too vague. 'MatchesSelectValue'?
Oh, nice one!


================
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]]
----------------
spatel wrote:
> lebedev.ri wrote:
> > 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 :/
> Does D66607 catch this, or am I inverting the relationship of the patches?
D66607 does fix regressions in this file, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66232