[AMDGPU] Forbid dynamic alloca on PAL ABI

[sebastian-ne]

Summary

The PAL ABI does not support dynamically allocated stack objects, so
error out if a compiler frontend created them.

[arsenm]

This doesn't make sense. If you can externally set the stack size as was claimed in a previous patch, you support dynamic allocas

[sebastian-ne]

In D153280#4432808, @arsenm wrote:

This doesn't make sense. If you can externally set the stack size as was claimed in a previous patch, you support dynamic allocas

If you are referring to D150609, graphics uses indirect calls (under some known circumstances). It doesn’t use or support dynamic allocas.

Due to a bug in the compiler frontend, allocas got shifted into a non-entry block, which segfaulted in combination with D150609, therefore this patch to make sure nobody tries to use them.
The patch to fix the frontend is in https://github.com/GPUOpen-Drivers/llpc/pull/2542 for reference.

[arsenm]

In D153280#4432839, @sebastian-ne wrote:

Due to a bug in the compiler frontend, allocas got shifted into a non-entry block, which segfaulted in combination with D150609, therefore this patch to make sure nobody tries to use them.

This is a bug, not an ABI constraint. Should fix the bug instead of producing an error.

[arsenm]

In D153280#4432938, @arsenm wrote:

In D153280#4432839, @sebastian-ne wrote:

Due to a bug in the compiler frontend, allocas got shifted into a non-entry block, which segfaulted in combination with D150609, therefore this patch to make sure nobody tries to use them.

This is a bug, not an ABI constraint. Should fix the bug instead of producing an error.

It's also not PAL specific. OpenMP had the same out-of-entry bug before and reported crashes and nobody ever chased them down.

[arsenm]

https://godbolt.org/z/x59xjes4s I think the tear down is just totally broken. It looks like only the allocation side was implemented

[sebastian-ne]

This is a bug, not an ABI constraint.

Unless the PAL ABI changes, there is no way for the compiler to tell the thing that allocates scratch (=PAL or the driver) that dynamically sized stack objects exist. That’s why I see this as an ABI constraint.
There is also no need for that because (as far as I am aware) no graphics API exposes the ability to create dynamically sized objects on the stack.

(+nhaehnle for more API knowledge, in case I’m forgetting something.)

It looks like only the allocation side was implemented

Interesting. Maybe we should have an unconditional error then :)

[arsenm]

In D153280#4433311, @sebastian-ne wrote:

This is a bug, not an ABI constraint.

Unless the PAL ABI changes, there is no way for the compiler to tell the thing that allocates scratch (=PAL or the driver) that dynamically sized stack objects exist.

If dynamic call stack works then dynamic alloca works. The common property is compile time unknown stack size which is all that matters. The runtime has no reason to know these objects exist

It looks like only the allocation side was implemented

Interesting. Maybe we should have an unconditional error then :)

This isn't special for dynamic stack. We'll have the same problem with variadic calls (which @JonChesterfield is looking into), so we should just fix this to restore the SP from the FP

[nhaehnle]

I tend to agree with Matt here. *Current* graphics APIs don't allow users to write code that uses dynamic allocas, but since the stack size can be set externally there's no fundamental reason why supporting them should be impossible.

What I'd suggest here is that we beef up the llvm-dialects verifier support a little to make it possible to add custom verifier code, and then checking for non-entry allocas could be added to that verifier exclusively in the frontend.

[sebastian-ne]

I see, I’ll close this then.

[arsenm]

I also don't think fixing this is all *that* hard. It might take an hour or 2 of staring at emitEpilog

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

if (Info.UsesFlatScratch && !MFI->hasFlatScratchInit() &&
        !hasAnyNonFlatUseOfReg(MRI, *TII, AMDGPU::FLAT_SCR_HI))) {
     Info.UsesFlatScratch = false;
   }
 
   Info.PrivateSegmentSize = FrameInfo.getStackSize();
 
   // Assume a big number if there are any unknown sized objects.
   Info.HasDynamicallySizedStack = FrameInfo.hasVarSizedObjects();
-  if (Info.HasDynamicallySizedStack)
+  if (Info.HasDynamicallySizedStack) {
     Info.PrivateSegmentSize += AssumedStackSizeForDynamicSizeObjects;
+    if (TM.getMCSubtargetInfo()->getTargetTriple().getOS() == Triple::AMDPAL) {
+      report_fatal_error(
+          "PAL ABI does not support dynamically sized stack objects");
+    }
+  }
 
   if (MFI->isStackRealigned())
     Info.PrivateSegmentSize += FrameInfo.getMaxAlign().value();
 
   Info.UsesVCC =
       MRI.isPhysRegUsed(AMDGPU::VCC_LO) || MRI.isPhysRegUsed(AMDGPU::VCC_HI);
 
   // If there are no calls, MachineRegisterInfo can tell us the used register

llvm/test/CodeGen/AMDGPU/amdpal-callable.ll

-; RUN: llc -mtriple=amdgcn--amdpal -mattr=-xnack -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,SDAG,GFX8 -enable-var-scope %s
-; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx900 -mattr=-xnack -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,SDAG,GFX9 -enable-var-scope %s
-; RUN: llc -global-isel -mtriple=amdgcn--amdpal -mattr=-xnack -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GISEL,GFX9 -enable-var-scope %s
+; RUN: llc -mtriple=amdgcn--amdpal -mattr=-xnack -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX8 -enable-var-scope %s
+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx900 -mattr=-xnack -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX9 -enable-var-scope %s
+; RUN: llc -global-isel -mtriple=amdgcn--amdpal -mattr=-xnack -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX9 -enable-var-scope %s
 
 declare amdgpu_gfx float @extern_func(float) #0
 declare amdgpu_gfx float @extern_func_many_args(<64 x float>) #0
 
 @funcptr = external hidden unnamed_addr addrspace(4) constant ptr, align 4
 
 define amdgpu_gfx float @no_stack(float %arg0) #0 {
   %add = fadd float %arg0, 1.0
define amdgpu_gfx float @multiple_stack(float %arg0) #0 {
   %add = fadd float %arg0, %val
   %stack2 = alloca float, i32 4, align 4, addrspace(5)
   store volatile float 2.0, ptr addrspace(5) %stack2
   %val2 = load volatile float, ptr addrspace(5) %stack2
   %add2 = fadd float %add, %val2
   ret float %add2
 }
 
-define amdgpu_gfx float @dynamic_stack(float %arg0) #0 {
-bb0:
-  %cmp = fcmp ogt float %arg0, 0.0
-  br i1 %cmp, label %bb1, label %bb2
-
-bb1:
-  %stack = alloca float, i32 4, align 4, addrspace(5)
-  store volatile float 2.0, ptr addrspace(5) %stack
-  %val = load volatile float, ptr addrspace(5) %stack
-  %add = fadd float %arg0, %val
-  br label %bb2
-
-bb2:
-  %res = phi float [ 0.0, %bb0 ], [ %add, %bb1 ]
-  ret float %res
-}
-
-define amdgpu_gfx float @dynamic_stack_loop(float %arg0) #0 {
-bb0:
-  br label %bb1
-
-bb1:
-  %ctr = phi i32 [ 0, %bb0 ], [ %newctr, %bb1 ]
-  %stack = alloca float, i32 4, align 4, addrspace(5)
-  store volatile float 2.0, ptr addrspace(5) %stack
-  %val = load volatile float, ptr addrspace(5) %stack
-  %add = fadd float %arg0, %val
-  %cmp = icmp sgt i32 %ctr, 0
-  %newctr = sub i32 %ctr, 1
-  br i1 %cmp, label %bb1, label %bb2
-
-bb2:
-  ret float %add
-}
-
 define amdgpu_gfx float @no_stack_call(float %arg0) #0 {
   %res = call amdgpu_gfx float @simple_stack(float %arg0)
   ret float %res
 }
 
 define amdgpu_gfx float @simple_stack_call(float %arg0) #0 {
   %stack = alloca float, i32 4, align 4, addrspace(5)
   store volatile float 2.0, ptr addrspace(5) %stack

 
 attributes #0 = { nounwind }
 
 ; GCN: amdpal.pipelines:
 ; GCN-NEXT:  - .registers:
 ; GCN-NEXT:      0x2e12 (COMPUTE_PGM_RSRC1): 0xaf01ca{{$}}
 ; GCN-NEXT:      0x2e13 (COMPUTE_PGM_RSRC2): 0x8001{{$}}
 ; GCN-NEXT:    .shader_functions:
-; GCN-NEXT:      dynamic_stack:
-; GCN-NEXT:        .lds_size:       0{{$}}
-; GCN-NEXT:        .sgpr_count:     0x28{{$}}
-; GCN-NEXT:        .stack_frame_size_in_bytes: 0x10{{$}}
-; SDAG-NEXT:        .vgpr_count:     0x2{{$}}
-; GISEL-NEXT:        .vgpr_count:     0x3{{$}}
-; GCN-NEXT:      dynamic_stack_loop:
-; GCN-NEXT:        .lds_size:       0{{$}}
-; SDAG-NEXT:        .sgpr_count:     0x25{{$}}
-; GISEL-NEXT:        .sgpr_count:     0x26{{$}}
-; GCN-NEXT:        .stack_frame_size_in_bytes: 0x10{{$}}
-; SDAG-NEXT:        .vgpr_count:     0x3{{$}}
-; GISEL-NEXT:        .vgpr_count:     0x4{{$}}
 ; GCN-NEXT:      multiple_stack:
 ; GCN-NEXT:        .lds_size:       0{{$}}
 ; GCN-NEXT:        .sgpr_count:     0x21{{$}}
 ; GCN-NEXT:        .stack_frame_size_in_bytes: 0x24{{$}}
 ; GCN-NEXT:        .vgpr_count:     0x3{{$}}
 ; GCN-NEXT:      no_stack:
 ; GCN-NEXT:        .lds_size:       0{{$}}
 ; GCN-NEXT:        .sgpr_count:     0x20{{$}}

llvm/test/CodeGen/AMDGPU/amdpal-no-dyn-alloca.ll

+; RUN: not --crash llc -mtriple=amdgcn--amdpal -mcpu=gfx1100 -o - %s 2>&1 | FileCheck %s
+
+; Check that dynamic allocas are an error with the amdpal ABI.
+
+; CHECK: LLVM ERROR: PAL ABI does not support dynamically sized stack objects
+
+define amdgpu_gfx float @dynamic_stack(float %arg0) #0 {
+bb0:
+  %cmp = fcmp ogt float %arg0, 0.0
+  br i1 %cmp, label %bb1, label %bb2
+
+bb1:
+  %stack = alloca float, i32 4, align 4, addrspace(5)
+  store volatile float 2.0, ptr addrspace(5) %stack
+  %val = load volatile float, ptr addrspace(5) %stack
+  %add = fadd float %arg0, %val
+  br label %bb2
+
+bb2:
+  %res = phi float [ 0.0, %bb0 ], [ %add, %bb1 ]
+  ret float %res
+}
+
+define amdgpu_gfx float @dynamic_stack_loop(float %arg0) #0 {
+bb0:
+  br label %bb1
+
+bb1:
+  %ctr = phi i32 [ 0, %bb0 ], [ %newctr, %bb1 ]
+  %stack = alloca float, i32 4, align 4, addrspace(5)
+  store volatile float 2.0, ptr addrspace(5) %stack
+  %val = load volatile float, ptr addrspace(5) %stack
+  %add = fadd float %arg0, %val
+  %cmp = icmp sgt i32 %ctr, 0
+  %newctr = sub i32 %ctr, 1
+  br i1 %cmp, label %bb1, label %bb2
+
+bb2:
+  ret float %add
+}