[GVN][NewGVN] Take call attributes into account in expressions #114545

dtcxzyw · 2024-11-01T14:53:42Z

Drop canBeReplacedBy and take call attributes into account in expressions.
Address comment #114011 (review).

llvm/lib/Transforms/Scalar/GVN.cpp

dtcxzyw · 2024-11-01T17:01:03Z

Compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=155fed8017812fe451720f79d34c8a7287c9a24d&to=a84bc7a133d7f56047acdf4230470ce4caf675f5&stat=instructions%3Au

llvmbot · 2024-11-01T17:02:18Z

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Drop canBeReplacedBy and take call attributes into account in expressions.
Address comment #114011 (review).

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

5 Files Affected:

(modified) llvm/include/llvm/Transforms/Scalar/GVNExpression.h (+6)
(modified) llvm/lib/Transforms/Scalar/GVN.cpp (+9-12)
(modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+14-20)
(modified) llvm/test/Transforms/GVN/pr113997.ll (+36)
(modified) llvm/test/Transforms/NewGVN/pr113997.ll (+36)

diff --git a/llvm/include/llvm/Transforms/Scalar/GVNExpression.h b/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
index 2433890d0df802..71437953640c9b 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVNExpression.h
@@ -315,6 +315,12 @@ class CallExpression final : public MemoryExpression {
     return EB->getExpressionType() == ET_Call;
   }
 
+  bool equals(const Expression &Other) const override;
+  bool exactlyEquals(const Expression &Other) const override {
+    return Expression::exactlyEquals(Other) &&
+           cast<CallExpression>(Other).Call == Call;
+  }
+
   // Debugging support
   void printInternal(raw_ostream &OS, bool PrintEType) const override {
     if (PrintEType)
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index ad9b1217089d7d..13bf0e7dfc87b4 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -143,6 +143,8 @@ struct llvm::GVNPass::Expression {
   Type *type = nullptr;
   SmallVector<uint32_t, 4> varargs;
 
+  AttributeList attrs;
+
   Expression(uint32_t o = ~2U) : opcode(o) {}
 
   bool operator==(const Expression &other) const {
@@ -154,6 +156,9 @@ struct llvm::GVNPass::Expression {
       return false;
     if (varargs != other.varargs)
       return false;
+    if (!attrs.isEmpty() && !other.attrs.isEmpty() &&
+        !attrs.intersectWith(type->getContext(), other.attrs).has_value())
+      return false;
     return true;
   }
 
@@ -364,6 +369,8 @@ GVNPass::Expression GVNPass::ValueTable::createExpr(Instruction *I) {
   } else if (auto *SVI = dyn_cast<ShuffleVectorInst>(I)) {
     ArrayRef<int> ShuffleMask = SVI->getShuffleMask();
     e.varargs.append(ShuffleMask.begin(), ShuffleMask.end());
+  } else if (auto *CB = dyn_cast<CallBase>(I)) {
+    e.attrs = CB->getAttributes();
   }
 
   return e;
@@ -2189,16 +2196,6 @@ 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);
@@ -2744,7 +2741,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 || !canBeReplacedBy(I, Repl)) {
+  if (!Repl) {
     // Failure, just remember this instance for future use.
     LeaderTable.insert(Num, I, I->getParent());
     return false;
@@ -3010,7 +3007,7 @@ bool GVNPass::performScalarPRE(Instruction *CurInst) {
 
     uint32_t TValNo = VN.phiTranslate(P, CurrentBlock, ValNo, *this);
     Value *predV = findLeader(P, TValNo);
-    if (!predV || !canBeReplacedBy(CurInst, predV)) {
+    if (!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 6800ad51cc0a8f..0cba8739441bcb 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -945,6 +945,18 @@ bool StoreExpression::equals(const Expression &Other) const {
   return true;
 }
 
+bool CallExpression::equals(const Expression &Other) const {
+  if (!MemoryExpression::equals(Other))
+    return false;
+
+  if (auto *RHS = dyn_cast<CallExpression>(&Other))
+    return Call->getAttributes()
+        .intersectWith(Call->getContext(), RHS->Call->getAttributes())
+        .has_value();
+
+  return false;
+}
+
 // Determine if the edge From->To is a backedge
 bool NewGVN::isBackedge(BasicBlock *From, BasicBlock *To) const {
   return From == To ||
@@ -3854,16 +3866,6 @@ 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
@@ -3973,8 +3975,6 @@ bool NewGVN::eliminateInstructions(Function &F) {
           MembersLeft.insert(Member);
           continue;
         }
-        if (!canBeReplacedBy(Member, Leader))
-          continue;
 
         LLVM_DEBUG(dbgs() << "Found replacement " << *(Leader) << " for "
                           << *Member << "\n");
@@ -4082,11 +4082,8 @@ 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 (!canBeReplacedBy(DefI, DominatingLeader))
-                    continue;
+                if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>()))
                   patchReplacementInstruction(DefI, DominatingLeader);
-                }
 
                 markInstructionForDeletion(DefI);
               }
@@ -4134,11 +4131,8 @@ bool NewGVN::eliminateInstructions(Function &F) {
           // 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 (!canBeReplacedBy(ReplacedInst, DominatingLeader))
-              continue;
+          if (!PI || DominatingLeader != PI->OriginalOp)
             patchReplacementInstruction(ReplacedInst, DominatingLeader);
-          }
 
           LLVM_DEBUG(dbgs()
                      << "Found replacement " << *DominatingLeader << " for "
diff --git a/llvm/test/Transforms/GVN/pr113997.ll b/llvm/test/Transforms/GVN/pr113997.ll
index 35e73b1a4439b3..6aebdb3a5dfac3 100644
--- a/llvm/test/Transforms/GVN/pr113997.ll
+++ b/llvm/test/Transforms/GVN/pr113997.ll
@@ -31,3 +31,39 @@ if.else:
 if.then:
   ret i1 false
 }
+
+; Make sure we don't merge these two users of the incompatible call pair.
+
+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:    [[CTPOP2INC:%.*]] = add i32 [[CTPOP2]], 1
+; CHECK-NEXT:    [[RES:%.*]] = icmp eq i32 [[CTPOP2INC]], 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
+}
diff --git a/llvm/test/Transforms/NewGVN/pr113997.ll b/llvm/test/Transforms/NewGVN/pr113997.ll
index a919c8c304b1b9..59dce09e89c887 100644
--- a/llvm/test/Transforms/NewGVN/pr113997.ll
+++ b/llvm/test/Transforms/NewGVN/pr113997.ll
@@ -31,3 +31,39 @@ if.else:
 if.then:
   ret i1 false
 }
+
+; Make sure we don't merge these two users of the incompatible call pair.
+
+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:    [[CTPOP2INC:%.*]] = add i32 [[CTPOP2]], 1
+; CHECK-NEXT:    [[RES:%.*]] = icmp eq i32 [[CTPOP2INC]], 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
+}

dtcxzyw · 2024-11-07T15:57:18Z

Ping.

nikic

LGTM. This is ugly but I don't have a better idea.

ronlieb · 2024-11-08T23:49:03Z

getting traceback, see attached
traceback.txt

dtcxzyw · 2024-11-09T01:32:31Z

getting traceback, see attached traceback.txt

Can you please provide a reproducer?

Change-Id: Ic2c288949986f1b75ca402911f3967cb0a106254 --- revert: Assertion `Success && "We should not be trying to sink callbases " "with non-intersectable attributes"' * 984bca9 [GVN][NewGVN] Take call attributes into account in expressions (llvm#114545) breaks smoke/flang-reduce Change-Id: I8fcc47f81753f5f621baafa24fb15a96bbc32632

ronlieb · 2024-11-09T03:58:39Z

yes, but it will be a day or two

relands: eea8b44 [GVN] Handle empty attrs in Expression == (llvm#115761) 984bca9 [GVN][NewGVN] Take call attributes into account in expressions (llvm#114545) Reverts: breaks comgr build 41e3919 [clang] Introduce diagnostics suppression mappings (llvm#112517) Change-Id: I3189bf1b5d66b980d2d711c3fe1d5fe217699bb1

…114545) Drop `canBeReplacedBy` and take call attributes into account in expressions. Address comment llvm#114011 (review).

dtcxzyw added 2 commits November 1, 2024 22:30

[GVN][NewGVN] Add pre-commit tests. NFC.

155fed8

[GVN][NewGVN] Take call attributes into account in expressions

468916f

dtcxzyw requested review from jayfoad, nikic, fhahn and goldsteinn November 1, 2024 14:53

dtcxzyw mentioned this pull request Nov 1, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

nikic reviewed Nov 1, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/GVN.cpp Outdated Show resolved Hide resolved

dtcxzyw added 2 commits November 1, 2024 23:47

[GVN] Remove optional.

6556d1b

[GVN] Fix empty check.

a84bc7a

dtcxzyw mentioned this pull request Nov 1, 2024

pre-commit: PR114545 dtcxzyw/llvm-opt-benchmark#1592

Closed

dtcxzyw marked this pull request as ready for review November 1, 2024 17:01

llvmbot added the llvm:transforms label Nov 1, 2024

nikic approved these changes Nov 7, 2024

View reviewed changes

dtcxzyw merged commit 984bca9 into llvm:main Nov 8, 2024
11 checks passed

dtcxzyw deleted the perf/gvn-call-attr-fix branch November 8, 2024 08:00

dtcxzyw mentioned this pull request Nov 11, 2024

[GVN] Handle empty attrs in Expression == #115761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GVN][NewGVN] Take call attributes into account in expressions #114545

[GVN][NewGVN] Take call attributes into account in expressions #114545

Uh oh!

dtcxzyw commented Nov 1, 2024

Uh oh!

Uh oh!

dtcxzyw commented Nov 1, 2024

Uh oh!

llvmbot commented Nov 1, 2024

Uh oh!

dtcxzyw commented Nov 7, 2024

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

ronlieb commented Nov 8, 2024

Uh oh!

dtcxzyw commented Nov 9, 2024

Uh oh!

ronlieb commented Nov 9, 2024

Uh oh!

Uh oh!

[GVN][NewGVN] Take call attributes into account in expressions #114545

[GVN][NewGVN] Take call attributes into account in expressions #114545

Uh oh!

Conversation

dtcxzyw commented Nov 1, 2024

Uh oh!

Uh oh!

dtcxzyw commented Nov 1, 2024

Uh oh!

llvmbot commented Nov 1, 2024

Uh oh!

dtcxzyw commented Nov 7, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronlieb commented Nov 8, 2024

Uh oh!

dtcxzyw commented Nov 9, 2024

Uh oh!

ronlieb commented Nov 9, 2024

Uh oh!

Uh oh!