[PATCH] D39766: [InstCombine] Teach visitICmpInst to not break integer absolute value idioms

[Craig Topper via Phabricator via llvm-commits]

craig.topper created this revision.

This patch adds an early out to visitICmpInst if we are looking at a compare as part of an integer absolute value idiom. Similar is already done for min/max.

In the particular case I observed in a benchmark we had an absolute value of a load from an indexed global. We simplified the compare using foldCmpLoadFromIndexedGlobal into a magic bit vector, a shift, and an and. But the load result was still used for the select and the negate part of the absolute valute idiom. So we overcomplicated the code and lost the ability to recognize it as an absolute value.

I've chosen a simpler case for the test here.


https://reviews.llvm.org/D39766

Files:
  lib/Transforms/InstCombine/InstCombineCompares.cpp
  test/Transforms/InstCombine/icmp.ll


Index: test/Transforms/InstCombine/icmp.ll
===================================================================
--- test/Transforms/InstCombine/icmp.ll
+++ test/Transforms/InstCombine/icmp.ll
@@ -3270,3 +3270,19 @@
   %c = icmp sgt i8 %b2, %a2
   ret i1 %c
 }
+
+; Make sure InstCombine doesn't try too hard to simplify the icmp and break the abs idiom
+define i32 @abs_preserve(i32 %x) {
+; CHECK-LABEL: @abs_preserve(
+; CHECK-NEXT:    [[A:%.*]] = shl nsw i32 [[X:%.*]], 1
+; CHECK-NEXT:    [[C:%.*]] = icmp sgt i32 [[A]], -1
+; CHECK-NEXT:    [[NEGA:%.*]] = sub i32 0, [[A]]
+; CHECK-NEXT:    [[ABS:%.*]] = select i1 [[C]], i32 [[A]], i32 [[NEGA]]
+; CHECK-NEXT:    ret i32 [[ABS]]
+;
+  %a = mul nsw i32 %x, 2
+  %c = icmp sge i32 %a, 0
+  %nega = sub i32 0, %a
+  %abs = select i1 %c, i32 %a, i32 %nega
+  ret i32 %abs
+}
Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -4462,11 +4462,28 @@
   // operands has at least one user besides the compare (the select),
   // which would often largely negate the benefit of folding anyway.
   if (I.hasOneUse())
-    if (SelectInst *SI = dyn_cast<SelectInst>(*I.user_begin()))
-      if ((SI->getOperand(1) == Op0 && SI->getOperand(2) == Op1) ||
-          (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1))
+    if (SelectInst *SI = dyn_cast<SelectInst>(*I.user_begin())) {
+      Value *TrueVal = SI->getTrueValue();
+      Value *FalseVal = SI->getFalseValue();
+      if ((TrueVal == Op0 && FalseVal == Op1) ||
+          (FalseVal == Op0 && TrueVal == Op1))
         return nullptr;
 
+      // Prevent disturbing an absolute value operation as well.
+      const APInt *C;
+      if (match(Op1, m_APInt(C))) {
+        if ((Op0 == TrueVal && match(FalseVal, m_Neg(m_Specific(Op0)))) ||
+            (Op0 == FalseVal && match(TrueVal, m_Neg(m_Specific(Op0))))) {
+          ICmpInst::Predicate Pred = I.getPredicate();
+          if ((Pred == ICmpInst::ICMP_SGT &&
+               (C->isNullValue() || C->isAllOnesValue())) ||
+              (Pred == ICmpInst::ICMP_SLT &&
+               (C->isNullValue() || C->isOneValue())))
+            return nullptr;
+        }
+      }
+    }
+
   // Do this after checking for min/max to prevent infinite looping.
   if (Instruction *Res = foldICmpWithZero(I))
     return Res;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39766.121996.patch
Type: text/x-patch
Size: 2468 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171107/d76b86ce/attachment.bin>