[IPO] Prevent removal of some convergent attr #134863

Keenuts · 2025-04-08T14:32:33Z

When a callee is marked as convergent, some targets like HLSL/SPIR-V add a convergent token to the call.
This is valid if both functions are marked as convergent.

ADCE/BDCE and other DCE passes were allowed to remove convergence intrinsics when their token were unused.
This meant a leaf function could lose all its convergence intrinsics. This would allow further optimization to remove the convergent attribute from the callee.
Issue was the caller was not updated, and we now had a convergence token attached to a call function calling a non-convergent function.

This commit limits the removal of the convergent attribute when only users are calls without convergence intrinsics.
If any other use is found (either a call to a convergent, or something else like taking the function address), the removal is prevented.

When a callee is marked as `convergent`, some targets like HLSL/SPIR-V add a convergent token to the call. This is valid if both functions are marked as `convergent`. ADCE/BDCE and other DCE passes were allowed to remove convergence intrinsics when their token were unused. This meant a leaf function could lose all its convergence intrinsics. This would allow further optimization to remove the `convergent` attribute from the callee. Issue was the caller was not updated, and we now had a convergence token attached to a call function calling a non-convergent function. This commit limits the removal of the `convergent` attribute when only users are calls without convergence intrinsics. If any other use is found (either a call to a convergent, or something else like taking the function address), the removal is prevented.

llvmbot · 2025-04-08T14:34:17Z

@llvm/pr-subscribers-llvm-transforms

Author: Nathan Gauër (Keenuts)

Changes

When a callee is marked as convergent, some targets like HLSL/SPIR-V add a convergent token to the call.
This is valid if both functions are marked as convergent.

ADCE/BDCE and other DCE passes were allowed to remove convergence intrinsics when their token were unused.
This meant a leaf function could lose all its convergence intrinsics. This would allow further optimization to remove the convergent attribute from the callee.
Issue was the caller was not updated, and we now had a convergence token attached to a call function calling a non-convergent function.

This commit limits the removal of the convergent attribute when only users are calls without convergence intrinsics.
If any other use is found (either a call to a convergent, or something else like taking the function address), the removal is prevented.

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

4 Files Affected:

(modified) llvm/lib/Transforms/IPO/FunctionAttrs.cpp (+16-1)
(added) llvm/test/Transforms/ADCE/convergence.ll (+46)
(added) llvm/test/Transforms/BDCE/convergence.ll (+47)
(modified) llvm/test/Transforms/FunctionAttrs/convergent.ll (+32)

diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index ef7989507c89f..104668ce2849e 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -1864,6 +1864,19 @@ static bool InstrBreaksNonConvergent(Instruction &I,
          !SCCNodes.contains(CB->getCalledFunction());
 }
 
+static bool FunctionRequiresConvergence(const Function &F) {
+  for (auto &Use : F.uses()) {
+    CallBase *CB = dyn_cast<CallBase>(Use.getUser());
+    if (!CB)
+      return true;
+
+    if (CB->getConvergenceControlToken())
+      return true;
+  }
+
+  return false;
+}
+
 /// Helper for NoUnwind inference predicate InstrBreaksAttribute.
 static bool InstrBreaksNonThrowing(Instruction &I, const SCCNodeSet &SCCNodes) {
   if (!I.mayThrow(/* IncludePhaseOneUnwind */ true))
@@ -1967,7 +1980,9 @@ static void inferConvergent(const SCCNodeSet &SCCNodes,
   AI.registerAttrInference(AttributeInferer::InferenceDescriptor{
       Attribute::Convergent,
       // Skip non-convergent functions.
-      [](const Function &F) { return !F.isConvergent(); },
+      [](const Function &F) {
+        return !F.isConvergent() || FunctionRequiresConvergence(F);
+      },
       // Instructions that break non-convergent assumption.
       [SCCNodes](Instruction &I) {
         return InstrBreaksNonConvergent(I, SCCNodes);
diff --git a/llvm/test/Transforms/ADCE/convergence.ll b/llvm/test/Transforms/ADCE/convergence.ll
new file mode 100644
index 0000000000000..0bc317e898bf3
--- /dev/null
+++ b/llvm/test/Transforms/ADCE/convergence.ll
@@ -0,0 +1,46 @@
+; RUN: opt %s -passes=adce -S | FileCheck %s
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define i32 @foo(i32 %a) #0 {
+define i32 @foo(i32 %a) #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.entry()
+  ret i32 %a
+}
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define void @bar() #0 {
+define void @bar() #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.anchor()
+  ret void
+}
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define void @baz() #0 {
+define void @baz() #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.entry()
+  br label %header
+
+header:
+; CHECK-NOT: %1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %0) ]
+  %1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %0) ]
+  br i1 true, label %body, label %exit
+
+body:
+  br label %header
+
+exit:
+  ret void
+}
+
+declare token @llvm.experimental.convergence.entry() #1
+declare token @llvm.experimental.convergence.anchor() #1
+declare token @llvm.experimental.convergence.loop() #1
+
+attributes #0 = { convergent }
+attributes #1 = { convergent nocallback nofree nosync nounwind willreturn memory(none) }
diff --git a/llvm/test/Transforms/BDCE/convergence.ll b/llvm/test/Transforms/BDCE/convergence.ll
new file mode 100644
index 0000000000000..417d6cc809fb1
--- /dev/null
+++ b/llvm/test/Transforms/BDCE/convergence.ll
@@ -0,0 +1,47 @@
+; RUN: opt %s -passes=bdce -S | FileCheck %s
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define i32 @foo(i32 %a) #0 {
+define i32 @foo(i32 %a) #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.entry()
+  ret i32 %a
+}
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define void @bar() #0 {
+define void @bar() #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.anchor()
+  ret void
+}
+
+; CHECK:      Function Attrs: convergent
+; CHECK-NEXT: define void @baz() #0 {
+define void @baz() #0 {
+entry:
+; CHECK-NOT: %0 = call token @llvm.experimental.convergence.entry()
+  %0 = call token @llvm.experimental.convergence.entry()
+  br label %header
+
+header:
+; CHECK-NOT: %1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %0) ]
+  %1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %0) ]
+  br i1 true, label %body, label %exit
+
+body:
+  br label %header
+
+exit:
+  ret void
+}
+
+declare token @llvm.experimental.convergence.entry() #1
+declare token @llvm.experimental.convergence.anchor() #1
+declare token @llvm.experimental.convergence.loop() #1
+
+attributes #0 = { convergent }
+attributes #1 = { convergent nocallback nofree nosync nounwind willreturn memory(none) }
+
diff --git a/llvm/test/Transforms/FunctionAttrs/convergent.ll b/llvm/test/Transforms/FunctionAttrs/convergent.ll
index fe8029d39d924..62e1183e58688 100644
--- a/llvm/test/Transforms/FunctionAttrs/convergent.ll
+++ b/llvm/test/Transforms/FunctionAttrs/convergent.ll
@@ -129,3 +129,35 @@ define i32 @noopt_friend() convergent {
   %a = call i32 @noopt()
   ret i32 0
 }
+
+; A function which should normally be stripped of its convergent attribute,
+; but because it's used in a controlled convergence call, the attribute
+; remains.
+; This could be improved by propagating the non-convergence outside of the
+; function, but for the time being, we stop when we encounter this scenario.
+define i32 @leaf_noconvergent_used() convergent {
+; CHECK: Function Attrs: convergent mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; CHECK-LABEL: define {{[^@]+}}@leaf_noconvergent_used
+; CHECK-SAME: () #[[ATTR7:[0-9]+]] {
+; CHECK-NEXT:    ret i32 0
+;
+  ret i32 0
+}
+
+define i32 @nonleaf_convergent() convergent {
+; CHECK: Function Attrs: convergent mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; CHECK-LABEL: define {{[^@]+}}@nonleaf_convergent
+; CHECK-SAME: () #[[ATTR7]] {
+; CHECK-NEXT:    [[TMP1:%.*]] = call token @llvm.experimental.convergence.entry()
+; CHECK-NEXT:    [[TMP2:%.*]] = call i32 @leaf_noconvergent_used() [ "convergencectrl"(token [[TMP1]]) ]
+; CHECK-NEXT:    ret i32 0
+;
+  %1 = call token @llvm.experimental.convergence.entry()
+  %2 = call i32 @leaf_noconvergent_used() [ "convergencectrl"(token %1) ]
+  ret i32 0
+}
+
+
+declare token @llvm.experimental.convergence.entry() #1
+
+attributes #1 = { convergent nocallback nofree nosync nounwind willreturn memory(none) }

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

llvm/test/Transforms/ADCE/convergence.ll

llvm/test/Transforms/FunctionAttrs/convergent.ll

arsenm · 2025-04-08T15:41:59Z

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

+      [](const Function &F) {
+        return !F.isConvergent() || FunctionRequiresConvergence(F);
+      },


Does FunctionAttrs run in a bottom up and top down phase? Ideally we wouldn't need to inspect the users of the function, and strip convergent from call sites when possible

Looks like the pass is running bottom-up, so if we drop convergence intrinsics in the callee, then we should technically be able to strip the control tokens in the caller. But this would go beyond the scope of this pass no?

ssahasra · 2025-04-11T02:58:05Z

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

@@ -1864,6 +1864,23 @@ static bool InstrBreaksNonConvergent(Instruction &I,
         !SCCNodes.contains(CB->getCalledFunction());
 }

+static bool FunctionRequiresConvergence(const Function *F) {


This is identical to FunctionCalledWithConvergenceToken. Can we move the definition to a common place, say Function::hasConvergentCalls()?

ssahasra · 2025-04-11T03:04:15Z

The issue mentioned in #134844 can be fixed by relaxing the verifier. Given that, this change will benefit greatly from being more aggressive: if a function does not have a call to entry intrinsic, then remove tokens from any calls to that function, and iterate.

arsenm · 2025-04-11T07:48:16Z

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

@@ -2902,6 +2902,22 @@ struct AANonConvergentImpl : public AANonConvergent {
  }
 };

+static bool FunctionCalledWithConvergenceToken(const Function *F) {


Start with lowercase

arsenm · 2025-04-11T07:49:45Z

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

+    if (CB->getConvergenceControlToken())
+      return true;
+  }
+  return false;


This needs to be a conservatively correct true. If the address is captured you do not know the users. If the function is externally visible, you also do not know the users.

arsenm

This needs to consider external call users, but we shouldn't really need to look at all the users of the function.

I'm wondering if it's even worth removing the convergent attribute. The design is just broken. If we're going to require it to avoid failing the verifier, we probably should just never strip it. So either remove the verifier check, or never remove convergent.

We would be in a more holistically correct place if we propagated noconvergent (we could do this even prior to changing the IR default, and have the confusing convergent + noconvergent combo)

nikic · 2025-04-11T08:03:32Z

I'd prefer if we relaxed the verifier instead of doing this change.

Before this commit, having a convergence token on a non-convergent call was considered to be an error. This commit relaxes this requirement and allows convergence tokens to be present on non-convergent calls. When such token is present, they have no effect as the underlying call is non-convergent. This allows passes like DCE to strip `convergent` attribute from functions for which all convergent operations have been stripped. When this happens, a convergence token can still exist in the call-site, causing the verifier to complain. Alternatives have been considered in llvm#134863 and llvm#134844.

shiltian · 2025-04-16T01:39:12Z

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

+    if (!CB)
+      continue;
+
+    // We are not called, just used as an argument.
+    if (CB->getCalledFunction() != F)
+      continue;


Suggested change

if (!CB)

continue;

// We are not called, just used as an argument.

if (CB->getCalledFunction() != F)

continue;

if (!CB || CB->getCalledFunction() != F)

continue;

Keenuts · 2025-04-16T11:02:16Z

Hi, thanks for the reviews!
I uploaded a 3rd alternative which seems to please everybody: modify the verifier to relax the check: #135794

Before this commit, having a convergence token on a non-convergent call was considered to be an error. This commit relaxes this requirement and allows convergence tokens to be present on non-convergent calls. When such token is present, they have no effect as the underlying call is non-convergent. This allows passes like DCE to strip `convergent` attribute from functions for which all convergent operations have been stripped. When this happens, a convergence token can still exist in the call-site, causing the verifier to complain. Alternatives have been considered in #134863 and #134844.

Keenuts · 2025-05-02T08:18:16Z

Closed in favor of #135794

Before this commit, having a convergence token on a non-convergent call was considered to be an error. This commit relaxes this requirement and allows convergence tokens to be present on non-convergent calls. When such token is present, they have no effect as the underlying call is non-convergent. This allows passes like DCE to strip `convergent` attribute from functions for which all convergent operations have been stripped. When this happens, a convergence token can still exist in the call-site, causing the verifier to complain. Alternatives have been considered in llvm#134863 and llvm#134844.

llvmbot added the llvm:transforms label Apr 8, 2025

Keenuts requested review from arsenm and s-perron April 8, 2025 15:32

arsenm reviewed Apr 8, 2025

View reviewed changes

arsenm requested review from jdoerfert and shiltian April 8, 2025 15:42

Keenuts added 2 commits April 8, 2025 17:56

replace vreg with named values

73601b7

generate full checks instead of check-not

05f601d

Keenuts mentioned this pull request Apr 9, 2025

[IR] Mark convergence intrins as has-side-effect #134844

Closed

fix attributor

486720a

ssahasra reviewed Apr 11, 2025

View reviewed changes

arsenm reviewed Apr 11, 2025

View reviewed changes

Keenuts mentioned this pull request Apr 15, 2025

[IR] Relax convergence requirements on call #135794

Merged

shiltian reviewed Apr 16, 2025

View reviewed changes

s-perron removed their request for review April 16, 2025 18:34

Keenuts closed this May 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IPO] Prevent removal of some convergent attr #134863

[IPO] Prevent removal of some convergent attr #134863

Uh oh!

Keenuts commented Apr 8, 2025

Uh oh!

llvmbot commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Apr 8, 2025

Uh oh!

Keenuts Apr 9, 2025

Uh oh!

ssahasra Apr 11, 2025

Uh oh!

ssahasra commented Apr 11, 2025

Uh oh!

arsenm Apr 11, 2025

Uh oh!

arsenm Apr 11, 2025

Uh oh!

arsenm left a comment

Uh oh!

nikic commented Apr 11, 2025

Uh oh!

shiltian Apr 16, 2025

Uh oh!

Keenuts commented Apr 16, 2025

Uh oh!

Keenuts commented May 2, 2025

Uh oh!

Uh oh!

[IPO] Prevent removal of some convergent attr #134863

[IPO] Prevent removal of some convergent attr #134863

Uh oh!

Conversation

Keenuts commented Apr 8, 2025

Uh oh!

llvmbot commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Keenuts Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

ssahasra Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

ssahasra commented Apr 11, 2025

Uh oh!

arsenm Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented Apr 11, 2025

Uh oh!

shiltian Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Keenuts commented Apr 16, 2025

Uh oh!

Keenuts commented May 2, 2025

Uh oh!

Uh oh!