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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1299
+
+  auto ConstantsAreElementWiseEqual = [](Constant *Cx, Value *Y) {
+    // Are they fully identical?
----------------
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.


================
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) ||
----------------
'Check' is a bit too vague. 'MatchesSelectValue'?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1329
+
+  // Else, lets check the constant we'd have with flipped-strictness predicate.
+  auto FlippedStrictness = getFlippedStrictnessPredicateAndConstant(Pred, C0);
----------------
Remove 'Else, lets' - the sentence reads fine to me as:
  // Check the constant we'd have with flipped-strictness predicate.


================
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]]
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66232