Skip to content
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

[GVN] Drop nsw/nuw flags when replacing the result of a with.overflow intrinsic with a overflowing binary operator #82935

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Feb 25, 2024

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 25, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Alive2: https://alive2.llvm.org/ce/z/gyL7mn
Fixes #82884.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+7-1)
  • (added) llvm/test/Transforms/GVN/pr82884.ll (+21)
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 1373f5f7f4490c..075eeb5b19fd2b 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -3369,11 +3369,17 @@ void llvm::patchReplacementInstruction(Instruction *I, Value *Repl) {
 
   // Patch the replacement so that it is not more restrictive than the value
   // being replaced.
+  WithOverflowInst *UnusedWO;
+  // When replacing the result of a llvm.*.with.overflow intrinsic with a
+  // overflowing binary operator, nuw/nsw flags may no longer hold.
+  if (isa<OverflowingBinaryOperator>(ReplInst) &&
+      match(I, m_ExtractValue<0>(m_WithOverflowInst(UnusedWO))))
+    ReplInst->dropPoisonGeneratingFlags();
   // Note that if 'I' is a load being replaced by some operation,
   // for example, by an arithmetic operation, then andIRFlags()
   // would just erase all math flags from the original arithmetic
   // operation, which is clearly not wanted and not needed.
-  if (!isa<LoadInst>(I))
+  else if (!isa<LoadInst>(I))
     ReplInst->andIRFlags(I);
 
   // FIXME: If both the original and replacement value are part of the
diff --git a/llvm/test/Transforms/GVN/pr82884.ll b/llvm/test/Transforms/GVN/pr82884.ll
new file mode 100644
index 00000000000000..71abafda60d93d
--- /dev/null
+++ b/llvm/test/Transforms/GVN/pr82884.ll
@@ -0,0 +1,21 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=gvn < %s | FileCheck %s
+
+; Make sure nsw/nuw flags are dropped.
+
+define i32 @pr82884(i32 %x) {
+; CHECK-LABEL: define i32 @pr82884(
+; CHECK-SAME: i32 [[X:%.*]]) {
+; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[X]], [[X]]
+; CHECK-NEXT:    call void @use(i32 [[MUL]])
+; CHECK-NEXT:    [[MUL2:%.*]] = call { i32, i1 } @llvm.smul.with.overflow.i32(i32 [[X]], i32 [[X]])
+; CHECK-NEXT:    ret i32 [[MUL]]
+;
+  %mul = mul nsw nuw i32 %x, %x
+  call void @use(i32 %mul)
+  %mul2 = call { i32, i1 } @llvm.smul.with.overflow.i32(i32 %x, i32 %x)
+  %ret = extractvalue { i32, i1 } %mul2, 0
+  ret i32 %ret
+}
+
+declare void @use(i32)

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 892b4be into llvm:main Feb 26, 2024
5 of 6 checks passed
@dtcxzyw dtcxzyw deleted the fix-gvn-merge-overflow branch February 26, 2024 07:55
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 26, 2024
… intrinsic with a overflowing binary operator (llvm#82935)

Alive2: https://alive2.llvm.org/ce/z/gyL7mn
Fixes llvm#82884.

(cherry picked from commit 892b4be)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 26, 2024
… intrinsic with a overflowing binary operator (llvm#82935)

Alive2: https://alive2.llvm.org/ce/z/gyL7mn
Fixes llvm#82884.

(cherry picked from commit 892b4be)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GVN] GVNPass forgets to remove poison generating flags
3 participants