AMDGPU: allow reordering of functions in AMDGPUResourceUsageAnalysis

[jweightman]

Summary

The AMDGPUResourceUsageAnalysis was previously a CGSCC pass, and assumed
that a function's callees were always analyzed prior to their callees.
When it was refactored into a module pass, this assumption no longer
always holds. This results in calls being erroneously identified as
indirect, and reserving private segment space for them. This results in
significantly slower kernel launch latency.

This patch changes the order in which the module's functions are analyzed
from the order in which they occur in the module to a post-order traversal
of the call graph. Perhaps Clang always generates the module's functions
in such an order, but this is not the case for the Cray Fortran compiler.

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-resource-usage-function-ordering.ll

2

Can you use the explicit external size flag so this test breaks when it’s removed?

23

Checking the stack size is 0 isn’t going to be a reliable check since we’re going to stop adding an arbitrary constant for unknown stack size and reporting dynamic stack separately. I’d prefer to have some register count checks too

[jweightman]

I updated the regression test to also check the behavior with external calls, and explicitly set the assumed external call stack size. I also included SGPR and VGPR register count assertions for all kernels in the test.

[jweightman]

llvm/test/CodeGen/AMDGPU/hsa-metadata-resource-usage-function-ordering.ll

23

@arsenm I added the register count checks, but I'm not sure how to make the stack size check more reliable at the moment. Could you clarify a few questions, so I can improve the test?

we’re going to stop adding an arbitrary constant for unknown stack size and reporting dynamic stack separately

This sounds like unknown calls will contribute 0 to the private segment fixed size, and there will be some other field that describes dynamic stack usage separately. What will this field be called? What values will it be able to have, and what will they mean?

Also, if this isn't implemented yet, how can I write a check for it or future-proof this test now?

[jweightman]

I saw that the test was failing on the automatic build, so it looks like the register counts may have changed recently. The register counts depend on the architecture for this test case, and I've updated the checks in the test to match.

[jweightman]

Ping @arsenm

[jweightman]

ping

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-resource-usage-function-ordering.ll

5

You can check the direct asm output. I don't see a reason to go through the disassembler here

23

The flag will be removed in the future, so simply using it should be enough since it will fail with unknown flag error.

[arsenm]

llvm/test/CodeGen/AMDGPU/hsa-metadata-resource-usage-function-ordering.ll

16–19

These aren't the most helpful register counts. Can you mark the functions internal, and add some artificial asm register uses to get a more helpful value?

[arsenm]

LGTM

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

 /// in the module.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "AMDGPUResourceUsageAnalysis.h"
 #include "AMDGPU.h"
 #include "GCNSubtarget.h"
 #include "SIMachineFunctionInfo.h"
+#include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Target/TargetMachine.h"
 
 using namespace llvm;
bool AMDGPUResourceUsageAnalysis::runOnModule(Module &M) {
   auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
   if (!TPC)
     return false;
 
   MachineModuleInfo &MMI = getAnalysis<MachineModuleInfoWrapperPass>().getMMI();
   const TargetMachine &TM = TPC->getTM<TargetMachine>();
   bool HasIndirectCall = false;
 
-  for (Function &F : M) {
-    if (F.isDeclaration())
+  CallGraph CG = CallGraph(M);
+  auto End = po_end(&CG);
+
+  for (auto IT = po_begin(&CG); IT != End; ++IT) {
+    Function *F = IT->getFunction();
+    if (!F || F->isDeclaration())
       continue;
 
-    MachineFunction *MF = MMI.getMachineFunction(F);
+    MachineFunction *MF = MMI.getMachineFunction(*F);
     assert(MF && "function must have been generated already");
 
     auto CI = CallGraphResourceInfo.insert(
-        std::make_pair(&F, SIFunctionResourceInfo()));
+        std::make_pair(F, SIFunctionResourceInfo()));
     SIFunctionResourceInfo &Info = CI.first->second;
     assert(CI.second && "should only be called once per function");
     Info = analyzeResourceUsage(*MF, TM);
     HasIndirectCall |= Info.HasIndirectCall;
   }
 
   if (HasIndirectCall)
     propagateIndirectCallRegisterUsage();

llvm/test/CodeGen/AMDGPU/hsa-metadata-resource-usage-function-ordering.ll

+; Note: uses a randomly selected assumed external call stack size so that the
+; test assertions are unlikely to succeed by accident.
+
+; RUN: llc -amdgpu-assume-external-call-stack-size=5310 -mtriple=amdgcn-amd-amdhsa -mcpu=gfx700 --amdhsa-code-object-version=4 -enable-misched=0 -filetype=asm -o - < %s | FileCheck --check-prefixes CHECK,GFX7 %s
+; RUN: llc -amdgpu-assume-external-call-stack-size=5310 -mattr=-xnack -mtriple=amdgcn-amd-amdhsa --amdhsa-code-object-version=4 -mcpu=gfx803 -enable-misched=0 -filetype=asm -o - < %s | FileCheck --check-prefixes CHECK,GFX8 %s
+; RUN: llc -amdgpu-assume-external-call-stack-size=5310 -mattr=-xnack -mtriple=amdgcn-amd-amdhsa --amdhsa-code-object-version=4 -mcpu=gfx900 -enable-misched=0 -filetype=asm -o - < %s | FileCheck --check-prefixes CHECK,GFX9 %s
+; RUN: llc -amdgpu-assume-external-call-stack-size=5310 -mattr=-xnack -mtriple=amdgcn-amd-amdhsa --amdhsa-code-object-version=4 -mcpu=gfx1010 -enable-misched=0 -filetype=asm -o - < %s | FileCheck --check-prefixes CHECK,GFX10 %s
+
+; CHECK-LABEL: amdhsa.kernels
+
+; test a kernel without an external call that occurs before its callee in the module
+; CHECK-LABEL: test1
+; CHECK:     .private_segment_fixed_size: 20
+
+; GFX7: .sgpr_count:     37
+; GFX7: .sgpr_spill_count: 0
+; GFX7: .vgpr_count:     4
+; GFX7: .vgpr_spill_count: 0
+
+; GFX8:     .sgpr_count:     39
+; GFX8:     .sgpr_spill_count: 0
+; GFX8:     .vgpr_count:     4
+; GFX8:     .vgpr_spill_count: 0
+
+; GFX9:     .sgpr_count:     39
+; GFX9:     .sgpr_spill_count: 0
+; GFX9:     .vgpr_count:     4
+; GFX9:     .vgpr_spill_count: 0
+
+; GFX10:     .sgpr_count:     33
+; GFX10:     .sgpr_spill_count: 0
+; GFX10:     .vgpr_count:     4
+; GFX10:     .vgpr_spill_count: 0
+define amdgpu_kernel void @test1(float* %x) {
+  %1 = load volatile float, float* %x
+  %2 = call float @f(float %1)
+  store volatile float %2, float* %x
+  ret void
+}
+
+define internal float @f(float %arg0) #0 {
+  %stack = alloca float, i32 4, align 4, addrspace(5)
+  store volatile float 3.0, float addrspace(5)* %stack
+  %val = load volatile float, float addrspace(5)* %stack
+  %add = fadd float %arg0, %val
+  ret float %add
+}
+
+; test a kernel without an external call that occurs after its callee in the module
+; CHECK-LABEL: test2
+; CHECK:     .private_segment_fixed_size: 20
+
+; GFX7:     .sgpr_count:     37
+; GFX7:     .sgpr_spill_count: 0
+; GFX7:     .vgpr_count:     4
+; GFX7:     .vgpr_spill_count: 0
+
+; GFX8:     .sgpr_count:     39
+; GFX8:     .sgpr_spill_count: 0
+; GFX8:     .vgpr_count:     4
+; GFX8:     .vgpr_spill_count: 0
+
+; GFX9:     .sgpr_count:     39
+; GFX9:     .sgpr_spill_count: 0
+; GFX9:     .vgpr_count:     4
+; GFX9:     .vgpr_spill_count: 0
+
+; GFX10:     .sgpr_count:     33
+; GFX10:     .sgpr_spill_count: 0
+; GFX10:     .vgpr_count:     4
+; GFX10:     .vgpr_spill_count: 0
+define amdgpu_kernel void @test2(float* %x) {
+  %1 = load volatile float, float* %x
+  %2 = call float @f(float %1)
+  store volatile float %2, float* %x
+  ret void
+}
+
+; test a kernel with an external call that occurs before its callee in the module
+; CHECK-LABEL: test3
+; CHECK:     .private_segment_fixed_size: 5310
+
+; GFX7:     .sgpr_count:     37
+; GFX7:     .sgpr_spill_count: 0
+; GFX7:     .vgpr_count:     32
+; GFX7:     .vgpr_spill_count: 0
+
+; GFX8:     .sgpr_count:     39
+; GFX8:     .sgpr_spill_count: 0
+; GFX8:     .vgpr_count:     32
+; GFX8:     .vgpr_spill_count: 0
+
+; GFX9:     .sgpr_count:     39
+; GFX9:     .sgpr_spill_count: 0
+; GFX9:     .vgpr_count:     32
+; GFX9:     .vgpr_spill_count: 0
+
+; GFX10:     .sgpr_count:     35
+; GFX10:     .sgpr_spill_count: 0
+; GFX10:     .vgpr_count:     32
+; GFX10:     .vgpr_spill_count: 0
+define amdgpu_kernel void @test3() {
+  call void @g()
+  ret void
+}
+
+declare void @g() #0
+
+; test a kernel without an external call that occurs after its callee in the module
+; CHECK-LABEL: test4
+; CHECK:     .private_segment_fixed_size: 5310
+
+; GFX7:     .sgpr_count:     37
+; GFX7:     .sgpr_spill_count: 0
+; GFX7:     .vgpr_count:     32
+; GFX7:     .vgpr_spill_count: 0
+
+; GFX8:     .sgpr_count:     39
+; GFX8:     .sgpr_spill_count: 0
+; GFX8:     .vgpr_count:     32
+; GFX8:     .vgpr_spill_count: 0
+
+; GFX9:     .sgpr_count:     39
+; GFX9:     .sgpr_spill_count: 0
+; GFX9:     .vgpr_count:     32
+; GFX9:     .vgpr_spill_count: 0
+
+; GFX10:     .sgpr_count:     35
+; GFX10:     .sgpr_spill_count: 0
+; GFX10:     .vgpr_count:     32
+; GFX10:     .vgpr_spill_count: 0
+define amdgpu_kernel void @test4() {
+  call void @g()
+  ret void
+}
+
+attributes #0 = { norecurse }