[AMDGPU] Remove duplicated/confusing helpers. NFCI #142598

rovka · 2025-06-03T13:02:36Z

Move canGuaranteeTCO and mayTailCallThisCC into AMDGPUBaseInfo instead of keeping two copies for DAG/Global ISel.

Also remove isKernelCC, which doesn't agree with isKernel and doesn't seem very useful.

Move canGuaranteeTCO and mayTailCallThisCC into AMDGPUBaseInfo instead of keeping two copies for DAG/Global ISel. Also remove isKernelCC, which doesn't agree with isKernel and doesn't seem very useful.

llvmbot · 2025-06-03T13:03:12Z

@llvm/pr-subscribers-backend-amdgpu

Author: Diana Picus (rovka)

Changes

Move canGuaranteeTCO and mayTailCallThisCC into AMDGPUBaseInfo instead of keeping two copies for DAG/Global ISel.

Also remove isKernelCC, which doesn't agree with isKernel and doesn't seem very useful.

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

6 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp (+3-18)
(modified) llvm/lib/Target/AMDGPU/AMDGPUMemoryUtils.cpp (-7)
(modified) llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp (+3-1)
(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+2-17)
(modified) llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp (+10-2)
(modified) llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h (+4-3)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp b/llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp
index 98a32f9225ba9..c01370b788062 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp
@@ -1090,22 +1090,6 @@ bool AMDGPUCallLowering::areCalleeOutgoingArgsTailCallable(
   return parametersInCSRMatch(MRI, CallerPreservedMask, OutLocs, OutArgs);
 }
 
-/// Return true if the calling convention is one that we can guarantee TCO for.
-static bool canGuaranteeTCO(CallingConv::ID CC) {
-  return CC == CallingConv::Fast;
-}
-
-/// Return true if we might ever do TCO for calls with this calling convention.
-static bool mayTailCallThisCC(CallingConv::ID CC) {
-  switch (CC) {
-  case CallingConv::C:
-  case CallingConv::AMDGPU_Gfx:
-    return true;
-  default:
-    return canGuaranteeTCO(CC);
-  }
-}
-
 bool AMDGPUCallLowering::isEligibleForTailCallOptimization(
     MachineIRBuilder &B, CallLoweringInfo &Info,
     SmallVectorImpl<ArgInfo> &InArgs, SmallVectorImpl<ArgInfo> &OutArgs) const {
@@ -1130,7 +1114,7 @@ bool AMDGPUCallLowering::isEligibleForTailCallOptimization(
   if (!CallerPreserved)
     return false;
 
-  if (!mayTailCallThisCC(CalleeCC)) {
+  if (!AMDGPU::mayTailCallThisCC(CalleeCC)) {
     LLVM_DEBUG(dbgs() << "... Calling convention cannot be tail called.\n");
     return false;
   }
@@ -1145,7 +1129,8 @@ bool AMDGPUCallLowering::isEligibleForTailCallOptimization(
 
   // If we have -tailcallopt, then we're done.
   if (MF.getTarget().Options.GuaranteedTailCallOpt)
-    return canGuaranteeTCO(CalleeCC) && CalleeCC == CallerF.getCallingConv();
+    return AMDGPU::canGuaranteeTCO(CalleeCC) &&
+           CalleeCC == CallerF.getCallingConv();
 
   // Verify that the incoming and outgoing arguments from the callee are
   // safe to tail call.
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUMemoryUtils.cpp b/llvm/lib/Target/AMDGPU/AMDGPUMemoryUtils.cpp
index 6568d9031987e..241dbd63eb5c0 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUMemoryUtils.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUMemoryUtils.cpp
@@ -125,13 +125,6 @@ void getUsesOfLDSByFunction(const CallGraph &CG, Module &M,
 }
 
 bool isKernelLDS(const Function *F) {
-  // Some weirdness here. AMDGPU::isKernelCC does not call into
-  // AMDGPU::isKernel with the calling conv, it instead calls into
-  // isModuleEntryFunction which returns true for more calling conventions
-  // than AMDGPU::isKernel does. There's a FIXME on AMDGPU::isKernel.
-  // There's also a test that checks that the LDS lowering does not hit on
-  // a graphics shader, denoted amdgpu_ps, so stay with the limited case.
-  // Putting LDS in the name of the function to draw attention to this.
   return AMDGPU::isKernel(F->getCallingConv());
 }
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
index f0a0c2113bf81..d59087839b0e1 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -978,7 +978,9 @@ bool AMDGPUTargetMachine::isNoopAddrSpaceCast(unsigned SrcAS,
 
 unsigned AMDGPUTargetMachine::getAssumedAddrSpace(const Value *V) const {
   if (auto *Arg = dyn_cast<Argument>(V);
-      Arg && AMDGPU::isKernelCC(Arg->getParent()) && !Arg->hasByRefAttr())
+      Arg &&
+      AMDGPU::isModuleEntryFunctionCC(Arg->getParent()->getCallingConv()) &&
+      !Arg->hasByRefAttr())
     return AMDGPUAS::GLOBAL_ADDRESS;
 
   const auto *LD = dyn_cast<LoadInst>(V);
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index 1957e442dbabb..97a249259b5b4 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -3574,21 +3574,6 @@ void SITargetLowering::passSpecialInputs(
   }
 }
 
-static bool canGuaranteeTCO(CallingConv::ID CC) {
-  return CC == CallingConv::Fast;
-}
-
-/// Return true if we might ever do TCO for calls with this calling convention.
-static bool mayTailCallThisCC(CallingConv::ID CC) {
-  switch (CC) {
-  case CallingConv::C:
-  case CallingConv::AMDGPU_Gfx:
-    return true;
-  default:
-    return canGuaranteeTCO(CC);
-  }
-}
-
 bool SITargetLowering::isEligibleForTailCallOptimization(
     SDValue Callee, CallingConv::ID CalleeCC, bool IsVarArg,
     const SmallVectorImpl<ISD::OutputArg> &Outs,
@@ -3597,7 +3582,7 @@ bool SITargetLowering::isEligibleForTailCallOptimization(
   if (AMDGPU::isChainCC(CalleeCC))
     return true;
 
-  if (!mayTailCallThisCC(CalleeCC))
+  if (!AMDGPU::mayTailCallThisCC(CalleeCC))
     return false;
 
   // For a divergent call target, we need to do a waterfall loop over the
@@ -3619,7 +3604,7 @@ bool SITargetLowering::isEligibleForTailCallOptimization(
   bool CCMatch = CallerCC == CalleeCC;
 
   if (DAG.getTarget().Options.GuaranteedTailCallOpt) {
-    if (canGuaranteeTCO(CalleeCC) && CCMatch)
+    if (AMDGPU::canGuaranteeTCO(CalleeCC) && CCMatch)
       return true;
     return false;
   }
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
index 1233973da140d..254b9f12280e5 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
@@ -2185,8 +2185,16 @@ bool isChainCC(CallingConv::ID CC) {
   }
 }
 
-bool isKernelCC(const Function *Func) {
-  return AMDGPU::isModuleEntryFunctionCC(Func->getCallingConv());
+bool canGuaranteeTCO(CallingConv::ID CC) { return CC == CallingConv::Fast; }
+
+bool mayTailCallThisCC(CallingConv::ID CC) {
+  switch (CC) {
+  case CallingConv::C:
+  case CallingConv::AMDGPU_Gfx:
+    return true;
+  default:
+    return canGuaranteeTCO(CC);
+  }
 }
 
 bool hasXNACK(const MCSubtargetInfo &STI) {
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
index e0534b2091f58..8820d2fc299d0 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
@@ -1329,9 +1329,6 @@ bool isModuleEntryFunctionCC(CallingConv::ID CC);
 LLVM_READNONE
 bool isChainCC(CallingConv::ID CC);
 
-bool isKernelCC(const Function *Func);
-
-// FIXME: Remove this when calling conventions cleaned up
 LLVM_READNONE
 inline bool isKernel(CallingConv::ID CC) {
   switch (CC) {
@@ -1343,6 +1340,10 @@ inline bool isKernel(CallingConv::ID CC) {
   }
 }
 
+/// Return true if we might ever do TCO for calls with this calling convention.
+bool mayTailCallThisCC(CallingConv::ID CC);
+bool canGuaranteeTCO(CallingConv::ID CC);
+
 bool hasXNACK(const MCSubtargetInfo &STI);
 bool hasSRAMECC(const MCSubtargetInfo &STI);
 bool hasMIMG_R128(const MCSubtargetInfo &STI);

arsenm · 2025-06-03T14:43:09Z

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

+      Arg &&
+      AMDGPU::isModuleEntryFunctionCC(Arg->getParent()->getCallingConv()) &&
+      !Arg->hasByRefAttr())


This is a functionality change which ideally would be tested

It's not intended to be a functionality change, I only inlined isKernelCC's confusing implementation...

llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp

arsenm · 2025-06-03T14:44:03Z

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

+bool canGuaranteeTCO(CallingConv::ID CC) { return CC == CallingConv::Fast; }
+
+bool mayTailCallThisCC(CallingConv::ID CC) {
+  switch (CC) {
+  case CallingConv::C:
+  case CallingConv::AMDGPU_Gfx:
+    return true;
+  default:
+    return canGuaranteeTCO(CC);
+  }


Might as well keep these in the header

Ok, but for consistency I'm moving all the others too. It's annoying to not be able to see all of them in one place.

shiltian · 2025-06-03T14:46:14Z

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

@@ -1343,6 +1340,10 @@ inline bool isKernel(CallingConv::ID CC) {
  }
 }

+/// Return true if we might ever do TCO for calls with this calling convention.
+bool mayTailCallThisCC(CallingConv::ID CC);


They look small enough to be a header only implementation. They can be a constexpr as well.

Yep, thanks.

rovka · 2025-06-05T09:18:07Z

CI failure looks like an unrelated timeout in lldb tests.

Move canGuaranteeTCO and mayTailCallThisCC into AMDGPUBaseInfo instead of keeping two copies for DAG/Global ISel. Also remove isKernelCC, which doesn't agree with isKernel and doesn't seem very useful. While at it, also move all the CC-related helpers into AMDGPUBaseInfo.h and mark them constexpr.

[AMDGPU] Remove duplicated/confusing helpers. NFCI

84692ce

Move canGuaranteeTCO and mayTailCallThisCC into AMDGPUBaseInfo instead of keeping two copies for DAG/Global ISel. Also remove isKernelCC, which doesn't agree with isKernel and doesn't seem very useful.

rovka requested review from jayfoad, arsenm, shiltian and mbrkusanin June 3, 2025 13:02

llvmbot added the backend:AMDGPU label Jun 3, 2025

arsenm approved these changes Jun 3, 2025

View reviewed changes

shiltian reviewed Jun 3, 2025

View reviewed changes

rovka added 2 commits June 4, 2025 15:16

Move all CC helpers to header + constexpr

fe7dfa5

Braces

d53435b

shiltian approved these changes Jun 4, 2025

View reviewed changes

arsenm approved these changes Jun 5, 2025

View reviewed changes

rovka merged commit 40a7dce into llvm:main Jun 5, 2025
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU] Remove duplicated/confusing helpers. NFCI #142598

[AMDGPU] Remove duplicated/confusing helpers. NFCI #142598

Uh oh!

rovka commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

arsenm Jun 3, 2025

Uh oh!

rovka Jun 4, 2025

Uh oh!

Uh oh!

arsenm Jun 3, 2025

Uh oh!

rovka Jun 4, 2025

Uh oh!

shiltian Jun 3, 2025

Uh oh!

rovka Jun 4, 2025

Uh oh!

rovka commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

[AMDGPU] Remove duplicated/confusing helpers. NFCI #142598

[AMDGPU] Remove duplicated/confusing helpers. NFCI #142598

Uh oh!

Conversation

rovka commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

arsenm Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

rovka Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arsenm Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

rovka Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

shiltian Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

rovka Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

rovka commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!