[PATCH] D71516: [InstCombine] Canonicalize select immediates

[Dave Green via Phabricator via llvm-commits]

dmgreen created this revision.
dmgreen added reviewers: spatel, lebedev.ri.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

I found a case where after a specific awkward set of inlining and combining, we would end up with code that looked like:

  %1 = icmp slt i32 %shr, -128
  %2 = select i1 %1, i32 128, i32 %shr
  %.inv = icmp sgt i32 %shr, 127
  %spec.select.i = select i1 %.inv, i32 127, i32 %2
  %conv7 = trunc i32 %spec.select.i to i8

This should be turned into a min/max pattern, but somewhere along the line the "-128" in the first select was instead transformed into "128", as only the bottom byte was ever demanded.

To fix this, I've put in further canonicalisation for the immediates of select, preferring to use the same value as the icmp if available.


https://reviews.llvm.org/D71516

Files:
  llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
  llvm/test/Transforms/InstCombine/select-imm-canon.ll


Index: llvm/test/Transforms/InstCombine/select-imm-canon.ll
===================================================================
--- llvm/test/Transforms/InstCombine/select-imm-canon.ll
+++ llvm/test/Transforms/InstCombine/select-imm-canon.ll
@@ -4,8 +4,8 @@
 define i8 @single(i32 %A) {
 ; CHECK-LABEL: @single(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[L1:%.*]] = icmp slt i32 [[A:%.*]], -128
-; CHECK-NEXT:    [[L2:%.*]] = select i1 [[L1]], i32 128, i32 [[A]]
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp sgt i32 [[A:%.*]], -128
+; CHECK-NEXT:    [[L2:%.*]] = select i1 [[TMP0]], i32 [[A]], i32 -128
 ; CHECK-NEXT:    [[CONV7:%.*]] = trunc i32 [[L2]] to i8
 ; CHECK-NEXT:    ret i8 [[CONV7]]
 ;
@@ -19,10 +19,10 @@
 define i8 @double(i32 %A) {
 ; CHECK-LABEL: @double(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[L1:%.*]] = icmp slt i32 [[A:%.*]], -128
-; CHECK-NEXT:    [[L2:%.*]] = select i1 [[L1]], i32 128, i32 [[A]]
-; CHECK-NEXT:    [[DOTINV:%.*]] = icmp sgt i32 [[A]], 127
-; CHECK-NEXT:    [[SPEC_SELECT_I:%.*]] = select i1 [[DOTINV]], i32 127, i32 [[L2]]
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp sgt i32 [[A:%.*]], -128
+; CHECK-NEXT:    [[L2:%.*]] = select i1 [[TMP0]], i32 [[A]], i32 -128
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp slt i32 [[L2]], 127
+; CHECK-NEXT:    [[SPEC_SELECT_I:%.*]] = select i1 [[TMP1]], i32 [[L2]], i32 127
 ; CHECK-NEXT:    [[CONV7:%.*]] = trunc i32 [[SPEC_SELECT_I]] to i8
 ; CHECK-NEXT:    ret i8 [[CONV7]]
 ;
@@ -39,7 +39,7 @@
 ; CHECK-LABEL: @thisdoesnotloop(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[L1:%.*]] = icmp slt i32 [[A:%.*]], -128
-; CHECK-NEXT:    [[L2:%.*]] = select i1 [[L1]], i32 128, i32 [[B:%.*]]
+; CHECK-NEXT:    [[L2:%.*]] = select i1 [[L1]], i32 -128, i32 [[B:%.*]]
 ; CHECK-NEXT:    [[CONV7:%.*]] = trunc i32 [[L2]] to i8
 ; CHECK-NEXT:    ret i8 [[CONV7]]
 ;
@@ -52,10 +52,10 @@
 
 define i8 @original(i32 %A, i32 %B) {
 ; CHECK-LABEL: @original(
-; CHECK-NEXT:    [[TMP1:%.*]] = icmp slt i32 [[A:%.*]], -128
-; CHECK-NEXT:    [[TMP2:%.*]] = select i1 [[TMP1]], i32 128, i32 [[A]]
-; CHECK-NEXT:    [[DOTINV:%.*]] = icmp sgt i32 [[A]], 127
-; CHECK-NEXT:    [[SPEC_SELECT_I:%.*]] = select i1 [[DOTINV]], i32 127, i32 [[TMP2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp sgt i32 [[A:%.*]], -128
+; CHECK-NEXT:    [[TMP2:%.*]] = select i1 [[TMP1]], i32 [[A]], i32 -128
+; CHECK-NEXT:    [[TMP3:%.*]] = icmp slt i32 [[TMP2]], 127
+; CHECK-NEXT:    [[SPEC_SELECT_I:%.*]] = select i1 [[TMP3]], i32 [[TMP2]], i32 127
 ; CHECK-NEXT:    [[CONV7:%.*]] = trunc i32 [[SPEC_SELECT_I]] to i8
 ; CHECK-NEXT:    ret i8 [[CONV7]]
 ;
Index: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -348,8 +348,35 @@
     assert(!LHSKnown.hasConflict() && "Bits known to be one AND zero?");
 
     // If the operands are constants, see if we can simplify them.
-    if (ShrinkDemandedConstant(I, 1, DemandedMask) ||
-        ShrinkDemandedConstant(I, 2, DemandedMask))
+    // This is similar to ShrinkDemandedConstant, but for a select we want to
+    // try and keep the selected constants the same as icmp value constants, if
+    // we can. This helps not break apart (or helps put back together)
+    // canonical patterns like min and max.
+    auto CanonicalizeSelectConstant = [](Instruction *I, unsigned OpNo,
+                                         APInt DemandedMask) {
+      const APInt *SelC;
+      if (!match(I->getOperand(OpNo), m_APInt(SelC)))
+        return false;
+
+      // Get the constant out of the ICmp, if there is one
+      const APInt *CmpC;
+      ICmpInst::Predicate Pred;
+      if (!match(I->getOperand(0), m_c_ICmp(Pred, m_APInt(CmpC), m_Value())) ||
+          CmpC->getBitWidth() != SelC->getBitWidth())
+        return ShrinkDemandedConstant(I, OpNo, DemandedMask);
+
+      // If the constant is already the same as the ICmp, leave it as-is.
+      if (*CmpC == *SelC)
+        return false;
+      // If the constants are not already the same, but can be with the demand
+      // mask, use the constant value from the ICmp.
+      if ((*CmpC & DemandedMask) != (*SelC & DemandedMask))
+        return ShrinkDemandedConstant(I, OpNo, DemandedMask);
+      I->setOperand(OpNo, ConstantInt::get(I->getType(), *CmpC));
+      return true;
+    };
+    if (CanonicalizeSelectConstant(I, 1, DemandedMask) ||
+        CanonicalizeSelectConstant(I, 2, DemandedMask))
       return I;
 
     // Only known if known in both the LHS and RHS.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71516.233942.patch
Type: text/x-patch
Size: 4619 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191214/0e16e452/attachment.bin>