Skip to content

[InstCombine] Drop UB-implying attrs/metadata after speculating an instruction #85542

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
Mar 17, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Mar 16, 2024

When speculating an instruction in InstCombinerImpl::FoldOpIntoSelect, the call may result in undefined behavior. This patch drops all UB-implying attrs/metadata to fix this.

Fixes #85536.

@dtcxzyw dtcxzyw requested a review from goldsteinn March 16, 2024 16:51
@dtcxzyw dtcxzyw requested a review from nikic as a code owner March 16, 2024 16:51
@llvmbot
Copy link
Member

llvmbot commented Mar 16, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

When speculating an instruction in InstCombinerImpl::FoldOpIntoSelect, the call may result in undefined behavior. This patch drops all UB-implying attrs/metadata to fix this.

Fixes #85536.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+7)
  • (added) llvm/test/Transforms/InstCombine/pr85536.ll (+44)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index edb046defbc1ca..3376653f48a5ca 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1650,6 +1650,13 @@ static Value *foldOperationIntoSelectOperand(Instruction &I, SelectInst *SI,
                                              Value *NewOp, InstCombiner &IC) {
   Instruction *Clone = I.clone();
   Clone->replaceUsesOfWith(SI, NewOp);
+
+  // If we speculated an instruction, we need to drop any metadata that may
+  // result in undefined behavior, as the metadata might have been valid
+  // only given the branch precondition.
+  // Similarly strip attributes on call parameters that may cause UB in
+  // location the call is moved to.
+  Clone->dropUBImplyingAttrsAndMetadata();
   IC.InsertNewInstBefore(Clone, SI->getIterator());
   return Clone;
 }
diff --git a/llvm/test/Transforms/InstCombine/pr85536.ll b/llvm/test/Transforms/InstCombine/pr85536.ll
new file mode 100644
index 00000000000000..5e2bab984f9f2b
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr85536.ll
@@ -0,0 +1,44 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+define i8 @test_drop_noundef(i1 %cond, i8 %val) {
+; CHECK-LABEL: define i8 @test_drop_noundef(
+; CHECK-SAME: i1 [[COND:%.*]], i8 [[VAL:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = call i8 @llvm.smin.i8(i8 [[VAL]], i8 0)
+; CHECK-NEXT:    [[RET:%.*]] = select i1 [[COND]], i8 -1, i8 [[TMP0]]
+; CHECK-NEXT:    ret i8 [[RET]]
+;
+entry:
+  %sel = select i1 %cond, i8 -1, i8 %val
+  %ret = call noundef i8 @llvm.smin.i8(i8 %sel, i8 0)
+  ret i8 %ret
+}
+
+define i1 @pr85536(i32 %a) {
+; CHECK-LABEL: define i1 @pr85536(
+; CHECK-SAME: i32 [[A:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ult i32 [[A]], 31
+; CHECK-NEXT:    [[SHL1:%.*]] = shl nsw i32 -1, [[A]]
+; CHECK-NEXT:    [[ZEXT:%.*]] = zext i32 [[SHL1]] to i64
+; CHECK-NEXT:    [[SHL2:%.*]] = shl i64 [[ZEXT]], 48
+; CHECK-NEXT:    [[SHR:%.*]] = ashr exact i64 [[SHL2]], 48
+; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @llvm.smin.i64(i64 [[SHR]], i64 0)
+; CHECK-NEXT:    [[TMP1:%.*]] = and i64 [[TMP0]], 65535
+; CHECK-NEXT:    [[RET1:%.*]] = icmp eq i64 [[TMP1]], 0
+; CHECK-NEXT:    [[RET:%.*]] = select i1 [[CMP1]], i1 [[RET1]], i1 false
+; CHECK-NEXT:    ret i1 [[RET]]
+;
+entry:
+  %cmp1 = icmp ugt i32 %a, 30
+  %shl1 = shl nsw i32 -1, %a
+  %zext = zext i32 %shl1 to i64
+  %shl2 = shl i64 %zext, 48
+  %shr = ashr exact i64 %shl2, 48
+  %sel = select i1 %cmp1, i64 -1, i64 %shr
+  %smin = call noundef i64 @llvm.smin.i64(i64 %sel, i64 0)
+  %masked = and i64 %smin, 65535
+  %ret = icmp eq i64 %masked, 0
+  ret i1 %ret
+}

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

@dtcxzyw dtcxzyw merged commit 252d019 into llvm:main Mar 17, 2024
4 checks passed
@dtcxzyw dtcxzyw deleted the fix-pr85536 branch March 17, 2024 06:15
@dtcxzyw dtcxzyw added this to the LLVM 18.X Release milestone Mar 17, 2024
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 17, 2024

/cherry-pick 252d019

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 17, 2024
…struction (llvm#85542)

When speculating an instruction in `InstCombinerImpl::FoldOpIntoSelect`,
the call may result in undefined behavior. This patch drops all
UB-implying attrs/metadata to fix this.

Fixes llvm#85536.

(cherry picked from commit 252d019)
@llvmbot
Copy link
Member

llvmbot commented Mar 17, 2024

/pull-request #85562

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.

[InstCombine] Miscompile at -O1
3 participants