[PATCH] D92717: [IC] Fix invalid creation of uadd.sat

[Dave Green via Phabricator via llvm-commits]

dmgreen created this revision.
dmgreen added reviewers: spatel, nikic, meheff, RKSimon.
Herald added a subscriber: hiraditya.
dmgreen requested review of this revision.
Herald added a project: LLVM.

We could create uadd.sat under incorrect circumstances if a select with -1 as the false value was canonicalized by swapping the T/F values. Unlike the other transforms in the same function, it is not invariant to equality.

Some alive proofs: https://alive2.llvm.org/ce/z/emmKKL
Fixes PR48390


https://reviews.llvm.org/D92717

Files:
  llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
  llvm/test/Transforms/InstCombine/saturating-add-sub.ll


Index: llvm/test/Transforms/InstCombine/saturating-add-sub.ll
===================================================================
--- llvm/test/Transforms/InstCombine/saturating-add-sub.ll
+++ llvm/test/Transforms/InstCombine/saturating-add-sub.ll
@@ -1722,8 +1722,10 @@
 
 define i32 @swapped_select(i32 %x1, i32 %x2) {
 ; CHECK-LABEL: @swapped_select(
-; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[X2:%.*]], i32 [[X1:%.*]])
-; CHECK-NEXT:    ret i32 [[TMP1]]
+; CHECK-NEXT:    [[X14:%.*]] = add i32 [[X1:%.*]], [[X2:%.*]]
+; CHECK-NEXT:    [[X6:%.*]] = icmp ugt i32 [[X14]], [[X2]]
+; CHECK-NEXT:    [[X9:%.*]] = select i1 [[X6]], i32 [[X14]], i32 -1
+; CHECK-NEXT:    ret i32 [[X9]]
 ;
   %x14 = add i32 %x1, %x2
   %x6 = icmp ult i32 %x2, %x14
@@ -1733,8 +1735,10 @@
 
 define i32 @swapped_both(i32 %x1, i32 %x2) {
 ; CHECK-LABEL: @swapped_both(
-; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @llvm.uadd.sat.i32(i32 [[X2:%.*]], i32 [[X1:%.*]])
-; CHECK-NEXT:    ret i32 [[TMP1]]
+; CHECK-NEXT:    [[X14:%.*]] = add i32 [[X1:%.*]], [[X2:%.*]]
+; CHECK-NEXT:    [[X6:%.*]] = icmp ugt i32 [[X14]], [[X2]]
+; CHECK-NEXT:    [[X9:%.*]] = select i1 [[X6]], i32 [[X14]], i32 -1
+; CHECK-NEXT:    ret i32 [[X9]]
 ;
   %x14 = add i32 %x1, %x2
   %x6 = icmp ugt i32 %x14, %x2
Index: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -768,9 +768,11 @@
   // Canonicalize -1 (saturated result) to true value of the select. Just
   // swapping the compare operands is legal, because the selected value is the
   // same in case of equality, so we can interchange u< and u<=.
+  bool Swapped = false;
   if (match(FVal, m_AllOnes())) {
     std::swap(TVal, FVal);
     std::swap(Cmp0, Cmp1);
+    Swapped = true;
   }
   if (!match(TVal, m_AllOnes()))
     return nullptr;
@@ -801,8 +803,9 @@
     return Builder.CreateBinaryIntrinsic(
         Intrinsic::uadd_sat, BO->getOperand(0), BO->getOperand(1));
   }
-  // The overflow may be detected via the add wrapping round.
-  if (match(Cmp0, m_c_Add(m_Specific(Cmp1), m_Value(Y))) &&
+  // The overflow may be detected via the add wrapping round. Which is only
+  // valid if we did not swap the true and false values.
+  if (!Swapped && match(Cmp0, m_c_Add(m_Specific(Cmp1), m_Value(Y))) &&
       match(FVal, m_c_Add(m_Specific(Cmp1), m_Specific(Y)))) {
     // ((X + Y) u< X) ? -1 : (X + Y) --> uadd.sat(X, Y)
     // ((X + Y) u< Y) ? -1 : (X + Y) --> uadd.sat(X, Y)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92717.309728.patch
Type: text/x-patch
Size: 2617 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201205/2708f4d2/attachment.bin>