Skip to content

[GVN][NewGVN][Local] Handle attributes for function calls after CSE #114011

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
Nov 1, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Oct 29, 2024

This patch intersects attributes of two calls to avoid introducing UB. It also skips incompatible call pairs in GVN/NewGVN. However, I cannot provide negative tests for these changes.

Fixes #113997.

@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch intersects attributes of two calls to avoid introducing UB. It also skips incompatible call pairs in GVN/NewGVN. However, I cannot provide negative tests for these changes.

Fixes #113997.


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

5 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/GVN.cpp (+12-2)
  • (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+25-5)
  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+11)
  • (added) llvm/test/Transforms/GVN/pr113997.ll (+33)
  • (added) llvm/test/Transforms/NewGVN/pr113997.ll (+33)
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 2ba600497e00d3..ad9b1217089d7d 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2189,6 +2189,16 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) {
   return Changed;
 }
 
+// Return true iff V1 can be replaced with V2.
+static bool canBeReplacedBy(Value *V1, Value *V2) {
+  if (auto *CB1 = dyn_cast<CallBase>(V1))
+    if (auto *CB2 = dyn_cast<CallBase>(V2))
+      return CB1->getAttributes()
+          .intersectWith(CB2->getContext(), CB2->getAttributes())
+          .has_value();
+  return true;
+}
+
 static void patchAndReplaceAllUsesWith(Instruction *I, Value *Repl) {
   patchReplacementInstruction(I, Repl);
   I->replaceAllUsesWith(Repl);
@@ -2734,7 +2744,7 @@ bool GVNPass::processInstruction(Instruction *I) {
   // Perform fast-path value-number based elimination of values inherited from
   // dominators.
   Value *Repl = findLeader(I->getParent(), Num);
-  if (!Repl) {
+  if (!Repl || !canBeReplacedBy(I, Repl)) {
     // Failure, just remember this instance for future use.
     LeaderTable.insert(Num, I, I->getParent());
     return false;
@@ -3000,7 +3010,7 @@ bool GVNPass::performScalarPRE(Instruction *CurInst) {
 
     uint32_t TValNo = VN.phiTranslate(P, CurrentBlock, ValNo, *this);
     Value *predV = findLeader(P, TValNo);
-    if (!predV) {
+    if (!predV || !canBeReplacedBy(CurInst, predV)) {
       predMap.push_back(std::make_pair(static_cast<Value *>(nullptr), P));
       PREPred = P;
       ++NumWithout;
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 13d9e8f186b47c..6800ad51cc0a8f 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3854,6 +3854,16 @@ Value *NewGVN::findPHIOfOpsLeader(const Expression *E,
   return nullptr;
 }
 
+// Return true iff V1 can be replaced with V2.
+static bool canBeReplacedBy(Value *V1, Value *V2) {
+  if (auto *CB1 = dyn_cast<CallBase>(V1))
+    if (auto *CB2 = dyn_cast<CallBase>(V2))
+      return CB1->getAttributes()
+          .intersectWith(CB2->getContext(), CB2->getAttributes())
+          .has_value();
+  return true;
+}
+
 bool NewGVN::eliminateInstructions(Function &F) {
   // This is a non-standard eliminator. The normal way to eliminate is
   // to walk the dominator tree in order, keeping track of available
@@ -3963,6 +3973,9 @@ bool NewGVN::eliminateInstructions(Function &F) {
           MembersLeft.insert(Member);
           continue;
         }
+        if (!canBeReplacedBy(Member, Leader))
+          continue;
+
         LLVM_DEBUG(dbgs() << "Found replacement " << *(Leader) << " for "
                           << *Member << "\n");
         auto *I = cast<Instruction>(Member);
@@ -4069,8 +4082,11 @@ bool NewGVN::eliminateInstructions(Function &F) {
               if (DominatingLeader != Def) {
                 // Even if the instruction is removed, we still need to update
                 // flags/metadata due to downstreams users of the leader.
-                if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>()))
+                if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>())) {
+                  if (!canBeReplacedBy(DefI, DominatingLeader))
+                    continue;
                   patchReplacementInstruction(DefI, DominatingLeader);
+                }
 
                 markInstructionForDeletion(DefI);
               }
@@ -4112,17 +4128,21 @@ bool NewGVN::eliminateInstructions(Function &F) {
           // Don't replace our existing users with ourselves.
           if (U->get() == DominatingLeader)
             continue;
-          LLVM_DEBUG(dbgs()
-                     << "Found replacement " << *DominatingLeader << " for "
-                     << *U->get() << " in " << *(U->getUser()) << "\n");
 
           // If we replaced something in an instruction, handle the patching of
           // metadata.  Skip this if we are replacing predicateinfo with its
           // original operand, as we already know we can just drop it.
           auto *ReplacedInst = cast<Instruction>(U->get());
           auto *PI = PredInfo->getPredicateInfoFor(ReplacedInst);
-          if (!PI || DominatingLeader != PI->OriginalOp)
+          if (!PI || DominatingLeader != PI->OriginalOp) {
+            if (!canBeReplacedBy(ReplacedInst, DominatingLeader))
+              continue;
             patchReplacementInstruction(ReplacedInst, DominatingLeader);
+          }
+
+          LLVM_DEBUG(dbgs()
+                     << "Found replacement " << *DominatingLeader << " for "
+                     << *U->get() << " in " << *(U->getUser()) << "\n");
           U->set(DominatingLeader);
           // This is now a use of the dominating leader, which means if the
           // dominating leader was dead, it's now live!
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 65c1669f92b4d3..47a70492559610 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -3508,6 +3508,17 @@ void llvm::patchReplacementInstruction(Instruction *I, Value *Repl) {
   else if (!isa<LoadInst>(I))
     ReplInst->andIRFlags(I);
 
+  // Handle attributes.
+  if (auto *CB1 = dyn_cast<CallBase>(ReplInst)) {
+    if (auto *CB2 = dyn_cast<CallBase>(I)) {
+      bool Success = CB1->tryIntersectAttributes(CB2);
+      assert(Success && "We should not be trying to sink callbases "
+                        "with non-intersectable attributes");
+      // For NDEBUG Compile.
+      (void)Success;
+    }
+  }
+
   // FIXME: If both the original and replacement value are part of the
   // same control-flow region (meaning that the execution of one
   // guarantees the execution of the other), then we can combine the
diff --git a/llvm/test/Transforms/GVN/pr113997.ll b/llvm/test/Transforms/GVN/pr113997.ll
new file mode 100644
index 00000000000000..35e73b1a4439b3
--- /dev/null
+++ b/llvm/test/Transforms/GVN/pr113997.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=gvn < %s | FileCheck %s
+
+; Make sure attributes in function calls are intersected correctly.
+
+define i1 @bucket(i32 noundef %x) {
+; CHECK-LABEL: define i1 @bucket(
+; CHECK-SAME: i32 noundef [[X:%.*]]) {
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp sgt i32 [[X]], 0
+; CHECK-NEXT:    [[CTPOP1:%.*]] = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 [[X]])
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp samesign ult i32 [[CTPOP1]], 2
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP1]], i1 [[CMP2]], i1 false
+; CHECK-NEXT:    br i1 [[COND]], label %[[IF_THEN:.*]], label %[[IF_ELSE:.*]]
+; CHECK:       [[IF_ELSE]]:
+; CHECK-NEXT:    [[RES:%.*]] = icmp eq i32 [[CTPOP1]], 1
+; CHECK-NEXT:    ret i1 [[RES]]
+; CHECK:       [[IF_THEN]]:
+; CHECK-NEXT:    ret i1 false
+;
+  %cmp1 = icmp sgt i32 %x, 0
+  %ctpop1 = tail call range(i32 1, 32) i32 @llvm.ctpop.i32(i32 %x)
+  %cmp2 = icmp samesign ult i32 %ctpop1, 2
+  %cond = select i1 %cmp1, i1 %cmp2, i1 false
+  br i1 %cond, label %if.then, label %if.else
+
+if.else:
+  %ctpop2 = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 %x)
+  %res = icmp eq i32 %ctpop2, 1
+  ret i1 %res
+
+if.then:
+  ret i1 false
+}
diff --git a/llvm/test/Transforms/NewGVN/pr113997.ll b/llvm/test/Transforms/NewGVN/pr113997.ll
new file mode 100644
index 00000000000000..a919c8c304b1b9
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr113997.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=newgvn < %s | FileCheck %s
+
+; Make sure attributes in function calls are intersected correctly.
+
+define i1 @bucket(i32 noundef %x) {
+; CHECK-LABEL: define i1 @bucket(
+; CHECK-SAME: i32 noundef [[X:%.*]]) {
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp sgt i32 [[X]], 0
+; CHECK-NEXT:    [[CTPOP1:%.*]] = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 [[X]])
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp samesign ult i32 [[CTPOP1]], 2
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP1]], i1 [[CMP2]], i1 false
+; CHECK-NEXT:    br i1 [[COND]], label %[[IF_THEN:.*]], label %[[IF_ELSE:.*]]
+; CHECK:       [[IF_ELSE]]:
+; CHECK-NEXT:    [[RES:%.*]] = icmp eq i32 [[CTPOP1]], 1
+; CHECK-NEXT:    ret i1 [[RES]]
+; CHECK:       [[IF_THEN]]:
+; CHECK-NEXT:    ret i1 false
+;
+  %cmp1 = icmp sgt i32 %x, 0
+  %ctpop1 = tail call range(i32 1, 32) i32 @llvm.ctpop.i32(i32 %x)
+  %cmp2 = icmp samesign ult i32 %ctpop1, 2
+  %cond = select i1 %cmp1, i1 %cmp2, i1 false
+  br i1 %cond, label %if.then, label %if.else
+
+if.else:
+  %ctpop2 = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 %x)
+  %res = icmp eq i32 %ctpop2, 1
+  ret i1 %res
+
+if.then:
+  ret i1 false
+}

@goldsteinn
Copy link
Contributor

This looks fine to me, although I think an easier way to implement this would be make patchReplacementInstruction return a bool to indicate I and Repl where successfully patched.

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Oct 29, 2024

an easier way to implement this would be make patchReplacementInstruction return a bool to indicate I and Repl where successfully patched.

I tried that before. But it may be too late in GVNPass::performScalarPRE :(

@pranavk
Copy link
Contributor

pranavk commented Oct 29, 2024

LGTM. I verified that this patch fixes our test case that was broken earlier.

Copy link
Contributor

@pranavk pranavk left a comment

Choose a reason for hiding this comment

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

LGTM but someone more knowledgable than me should also take a look.

@asmok-g
Copy link

asmok-g commented Oct 30, 2024

We need this to unblock us in google. When can we expect a merge ?

@dtcxzyw dtcxzyw requested a review from jayfoad October 30, 2024 11:19
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Oct 31, 2024

We need this to unblock us in google. When can we expect a merge ?

I will land this patch tomorrow if no more comments.

@dtcxzyw dtcxzyw merged commit f16bff1 into llvm:main Nov 1, 2024
10 checks passed
@dtcxzyw dtcxzyw deleted the perf/fix-pr113997 branch November 1, 2024 04:44
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.

The general implementation approach here looks incorrect to me. You can't really restrict replacements after the fact in GVN, you need to do so during value numbering.

Consider this minor variation of the test case:

define i1 @bucket2(i32 noundef %x) {
; CHECK-LABEL: define i1 @bucket2(
; CHECK-SAME: i32 noundef [[X:%.*]]) {
; CHECK-NEXT:    [[CMP1:%.*]] = icmp sgt i32 [[X]], 0
; CHECK-NEXT:    [[CTPOP1:%.*]] = tail call range(i32 1, 32) i32 @llvm.ctpop.i32(i32 zeroext [[X]])
; CHECK-NEXT:    [[CTPOP1INC:%.*]] = add i32 [[CTPOP1]], 1
; CHECK-NEXT:    [[CMP2:%.*]] = icmp samesign ult i32 [[CTPOP1INC]], 3
; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP1]], i1 [[CMP2]], i1 false
; CHECK-NEXT:    br i1 [[COND]], label %[[IF_THEN:.*]], label %[[IF_ELSE:.*]]
; CHECK:       [[IF_ELSE]]:
; CHECK-NEXT:    [[CTPOP2:%.*]] = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 [[X]])
; CHECK-NEXT:    [[RES:%.*]] = icmp eq i32 [[CTPOP1INC]], 2
; CHECK-NEXT:    ret i1 [[RES]]
; CHECK:       [[IF_THEN]]:
; CHECK-NEXT:    ret i1 false
;
  %cmp1 = icmp sgt i32 %x, 0
  %ctpop1 = tail call range(i32 1, 32) i32 @llvm.ctpop.i32(i32 zeroext %x)
  %ctpop1inc = add i32 %ctpop1, 1
  %cmp2 = icmp samesign ult i32 %ctpop1inc, 3
  %cond = select i1 %cmp1, i1 %cmp2, i1 false
  br i1 %cond, label %if.then, label %if.else

if.else:
  %ctpop2 = tail call range(i32 0, 33) i32 @llvm.ctpop.i32(i32 %x)
  %ctpop2inc = add i32 %ctpop2, 1
  %res = icmp eq i32 %ctpop2inc, 2
  ret i1 %res

if.then:
  ret i1 false
}

This just adds a zeroext dummy attribute to make sure the two ctpop calls are not combined directly, and an extra add 1 after them. Note that the adds will get CSEd, because the canBeReplacedBy check is specifically on the calls, not on any later instructions that are numbered based on the incorrect assumption that the calls are equivalent.

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Nov 1, 2024

You can't really restrict replacements after the fact in GVN, you need to do so during value numbering.

I've considered this approach before. But it is too heavy to add Attributes to GVNPass::Expression.

Note that the adds will get CSEd, because the canBeReplacedBy check is specifically on the calls, not on any later instructions that are numbered based on the incorrect assumption that the calls are equivalent.

You are right. I missed this case.

@nikic
Copy link
Contributor

nikic commented Nov 1, 2024

You can't really restrict replacements after the fact in GVN, you need to do so during value numbering.

I've considered this approach before. But it is too heavy to add Attributes to GVNPass::Expression.

I don't have any particularly good ideas on how to fix this properly. Given that this is a CSE transform, I think we could get away with only dropping the poison-generating attributes, and otherwise keeping the attributes of the first call, to avoid the problematic !canBeReplacedBy case.

smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
…lvm#114011)

This patch intersects attributes of two calls to avoid introducing UB.
It also skips incompatible call pairs in GVN/NewGVN. However, I cannot
provide negative tests for these changes.

Fixes llvm#113997.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…lvm#114011)

This patch intersects attributes of two calls to avoid introducing UB.
It also skips incompatible call pairs in GVN/NewGVN. However, I cannot
provide negative tests for these changes.

Fixes llvm#113997.
dtcxzyw added a commit that referenced this pull request Nov 8, 2024
Drop `canBeReplacedBy` and take call attributes into account in
expressions.
Address comment
#114011 (review).
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…114545)

Drop `canBeReplacedBy` and take call attributes into account in
expressions.
Address comment
llvm#114011 (review).
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.

[GVNPass] Range attribute should be handled after CSE
7 participants