release/19.x: [SLP]Check that operand of abs does not overflow before making it part of minbitwidth transformation

[llvmbot]

Backport 825f9cb 709abac

Requested by: @alexey-bataev

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (llvmbot)

Changes

Backport 825f9cb 709abac

Requested by: @alexey-bataev

---

Full diff: https://github.com/llvm/llvm-project/pull/113146.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+16)
• (added) llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll (+33)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index ab2b96cdc42db8..746ba51a981fe0 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -15440,9 +15440,25 @@ bool BoUpSLP::collectValuesToDemote(
                 MaskedValueIsZero(I->getOperand(1), Mask, SimplifyQuery(*DL)));
       });
     };
+    auto AbsChecker = [&](unsigned BitWidth, unsigned OrigBitWidth) {
+      assert(BitWidth <= OrigBitWidth && "Unexpected bitwidths!");
+      return all_of(E.Scalars, [&](Value *V) {
+        auto *I = cast<Instruction>(V);
+        unsigned SignBits = OrigBitWidth - BitWidth;
+        APInt Mask = APInt::getBitsSetFrom(OrigBitWidth, BitWidth - 1);
+        unsigned Op0SignBits =
+            ComputeNumSignBits(I->getOperand(0), *DL, 0, AC, nullptr, DT);
+        return SignBits <= Op0SignBits &&
+               ((SignBits != Op0SignBits &&
+                 !isKnownNonNegative(I->getOperand(0), SimplifyQuery(*DL))) ||
+                MaskedValueIsZero(I->getOperand(0), Mask, SimplifyQuery(*DL)));
+      });
+    };
     if (ID != Intrinsic::abs) {
       Operands.push_back(getOperandEntry(&E, 1));
       CallChecker = CompChecker;
+    } else {
+      CallChecker = AbsChecker;
     }
     InstructionCost BestCost =
         std::numeric_limits<InstructionCost::CostType>::max();
diff --git a/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll b/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll
new file mode 100644
index 00000000000000..51b635837d3b59
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S --passes=slp-vectorizer < %s | FileCheck %s
+
+define i32 @test(i32 %n) {
+; CHECK-LABEL: define i32 @test(
+; CHECK-SAME: i32 [[N:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[TMP0:%.*]] = insertelement <2 x i32> poison, i32 [[N]], i32 0
+; CHECK-NEXT:    [[TMP1:%.*]] = shufflevector <2 x i32> [[TMP0]], <2 x i32> poison, <2 x i32> zeroinitializer
+; CHECK-NEXT:    [[TMP2:%.*]] = add <2 x i32> [[TMP1]], <i32 1, i32 2>
+; CHECK-NEXT:    [[TMP3:%.*]] = zext <2 x i32> [[TMP2]] to <2 x i64>
+; CHECK-NEXT:    [[TMP7:%.*]] = mul nuw nsw <2 x i64> [[TMP3]], <i64 273837369, i64 273837369>
+; CHECK-NEXT:    [[TMP8:%.*]] = call <2 x i64> @llvm.abs.v2i64(<2 x i64> [[TMP7]], i1 true)
+; CHECK-NEXT:    [[TMP4:%.*]] = trunc <2 x i64> [[TMP8]] to <2 x i32>
+; CHECK-NEXT:    [[TMP5:%.*]] = extractelement <2 x i32> [[TMP4]], i32 0
+; CHECK-NEXT:    [[TMP6:%.*]] = extractelement <2 x i32> [[TMP4]], i32 1
+; CHECK-NEXT:    [[RES1:%.*]] = add i32 [[TMP5]], [[TMP6]]
+; CHECK-NEXT:    ret i32 [[RES1]]
+;
+entry:
+  %n1 = add i32 %n, 1
+  %zn1 = zext nneg i32 %n1 to i64
+  %m1 = mul nuw nsw i64 %zn1, 273837369
+  %a1 = call i64 @llvm.abs.i64(i64 %m1, i1 true)
+  %t1 = trunc i64 %a1 to i32
+  %n2 = add i32 %n, 2
+  %zn2 = zext nneg i32 %n2 to i64
+  %m2 = mul nuw nsw i64 %zn2, 273837369
+  %a2 = call i64 @llvm.abs.i64(i64 %m2, i1 true)
+  %t2 = trunc i64 %a2 to i32
+  %res1 = add i32 %t1, %t2
+  ret i32 %res1
+}

[tru]

who can review this? @nikic?

[nikic]

I don't understand this part of the condition. What is the meaning of SignBits != Op0SignBits && !isKnownNonNegative? Should this be SignBits != Op0SignBits || isKnownNonNegative?

Though I'm not really sure why we'd be interested in handling the case where the abs is known non-negative (including also the MaskedValueIsZero check below): If it's non-negative, wouldn't we expect the abs to fold away anyway?

[alexey-bataev]

Right, isKnownNonNegative is meaningless here. Instead we check that the value is negative and SignBits < Op0SignBits, i.e. we do not lose signedness info when trying to truncate the operand value

[nikic]

Should this then check isKnownNegative rather than !isKnownNonNegative? They're not the same. I'm still confused by this code.

[alexey-bataev]

!isKnownNonNegative is correct here

[tru]

What's the status of this one? Can it / should it be merged?

[tru]

Ping on this again. Are you happy with this now @nikic ?

[github-actions]

@alexey-bataev (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.