Skip to content

PR for llvm/llvm-project#78621 #80260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Feb 1, 2024

resolves #78621

@llvmbot
Copy link
Member Author

llvmbot commented Feb 1, 2024

@llvm/pr-subscribers-llvm-transforms

Author: None (llvmbot)

Changes

resolves llvm/llvm-project#78621


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+6-1)
  • (modified) llvm/test/Transforms/ConstraintElimination/minmax.ll (+3-1)
  • (added) llvm/test/Transforms/ConstraintElimination/umin-result-may-be-poison.ll (+62)
diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 8f09569d0d9cc..7b672e89b67aa 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1061,11 +1061,16 @@ void State::addInfoFor(BasicBlock &BB) {
           FactOrCheck::getCheck(DT.getNode(&BB), cast<CallInst>(&I)));
       break;
     // Enqueue the intrinsics to add extra info.
-    case Intrinsic::abs:
     case Intrinsic::umin:
     case Intrinsic::umax:
     case Intrinsic::smin:
     case Intrinsic::smax:
+      // TODO: Check if it is possible to instead only added the min/max facts
+      // when simplifying uses of the min/max intrinsics.
+      if (!isGuaranteedNotToBePoison(&I))
+        break;
+      [[fallthrough]];
+    case Intrinsic::abs:
       WorkList.push_back(FactOrCheck::getInstFact(DT.getNode(&BB), &I));
       break;
     }
diff --git a/llvm/test/Transforms/ConstraintElimination/minmax.ll b/llvm/test/Transforms/ConstraintElimination/minmax.ll
index a31cf6845ad67..82b932f14c4ff 100644
--- a/llvm/test/Transforms/ConstraintElimination/minmax.ll
+++ b/llvm/test/Transforms/ConstraintElimination/minmax.ll
@@ -306,7 +306,9 @@ define i1 @smin_branchless(i32 %x, i32 %y) {
 ; CHECK-SAME: (i32 [[X:%.*]], i32 [[Y:%.*]]) {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[MIN:%.*]] = call i32 @llvm.smin.i32(i32 [[X]], i32 [[Y]])
-; CHECK-NEXT:    [[RET:%.*]] = xor i1 true, false
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp sle i32 [[MIN]], [[X]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp sgt i32 [[MIN]], [[X]]
+; CHECK-NEXT:    [[RET:%.*]] = xor i1 [[CMP1]], [[CMP2]]
 ; CHECK-NEXT:    ret i1 [[RET]]
 ;
 entry:
diff --git a/llvm/test/Transforms/ConstraintElimination/umin-result-may-be-poison.ll b/llvm/test/Transforms/ConstraintElimination/umin-result-may-be-poison.ll
new file mode 100644
index 0000000000000..6d1d95ec4fdba
--- /dev/null
+++ b/llvm/test/Transforms/ConstraintElimination/umin-result-may-be-poison.ll
@@ -0,0 +1,62 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -p constraint-elimination -S %s | FileCheck %s
+
+; Tests for https://github.com/llvm/llvm-project/issues/78621.
+
+define i1 @umin_not_used(i32 %arg) {
+; CHECK-LABEL: define i1 @umin_not_used(
+; CHECK-SAME: i32 [[ARG:%.*]]) {
+; CHECK-NEXT:    [[ICMP:%.*]] = icmp slt i32 [[ARG]], 0
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @llvm.umin.i32(i32 [[SHL]], i32 80)
+; CHECK-NEXT:    [[CMP2:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    ret i1 [[ICMP]]
+;
+  %icmp = icmp slt i32 %arg, 0
+  %shl = shl nuw nsw i32 %arg, 3
+  call i32 @llvm.umin.i32(i32 %shl, i32 80)
+  %cmp2 = shl nuw nsw i32 %arg, 3
+  ret i1 %icmp
+}
+
+define i1 @umin_poison_is_UB_via_call(i32 %arg) {
+; CHECK-LABEL: define i1 @umin_poison_is_UB_via_call(
+; CHECK-SAME: i32 [[ARG:%.*]]) {
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    [[MIN:%.*]] = call i32 @llvm.umin.i32(i32 [[SHL]], i32 80)
+; CHECK-NEXT:    call void @noundef(i32 noundef [[MIN]])
+; CHECK-NEXT:    [[CMP2:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    ret i1 false
+;
+  %icmp = icmp slt i32 %arg, 0
+  %shl = shl nuw nsw i32 %arg, 3
+  %min = call i32 @llvm.umin.i32(i32 %shl, i32 80)
+  call void @noundef(i32 noundef %min)
+  %cmp2 = shl nuw nsw i32 %arg, 3
+  ret i1 %icmp
+}
+
+define i1 @umin_poison_call_before_UB(i32 %arg) {
+; CHECK-LABEL: define i1 @umin_poison_call_before_UB(
+; CHECK-SAME: i32 [[ARG:%.*]]) {
+; CHECK-NEXT:    [[ICMP:%.*]] = icmp slt i32 [[ARG]], 0
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    [[MIN:%.*]] = call i32 @llvm.umin.i32(i32 [[SHL]], i32 80)
+; CHECK-NEXT:    call void @fn()
+; CHECK-NEXT:    call void @noundef(i32 noundef [[MIN]])
+; CHECK-NEXT:    [[CMP2:%.*]] = shl nuw nsw i32 [[ARG]], 3
+; CHECK-NEXT:    ret i1 [[ICMP]]
+;
+  %icmp = icmp slt i32 %arg, 0
+  %shl = shl nuw nsw i32 %arg, 3
+  %min = call i32 @llvm.umin.i32(i32 %shl, i32 80)
+  call void @fn()
+  call void @noundef(i32 noundef %min)
+  %cmp2 = shl nuw nsw i32 %arg, 3
+  ret i1 %icmp
+}
+
+declare i32 @llvm.umin.i32(i32, i32) #0
+
+declare void @noundef(i32 noundef)
+declare void @fn()

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fhahn added 2 commits February 6, 2024 16:05
Tests with umin where the result may be poison for
llvm#78621.

(cherry picked from commit c83180c)
The result of umin may be poison and in that case the added constraints
are not be valid in contexts where poison doesn't cause UB. Only queue
facts for min/max intrinsics if the result is guaranteed to not be
poison.

This could be improved in the future, by only adding the fact when
solving conditions using the result value.

Fixes llvm#78621.

(cherry picked from commit 3d91d96)
@tstellar tstellar merged commit 56c50a4 into llvm:release/18.x Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants