AMDGPU: Turn off force init 16 input SGPRS for pal

[dstuttard]

Summary

Pal uses a different mechanism for user sgprs.

[arsenm]

I don't see why PAL would get special treatment here. I think the problem is we don't really try to account for the formal arguments in the user SGPR count. A better fix would be to track that accurately

[dstuttard]

In D129566#3645692, @arsenm wrote:

I don't see why PAL would get special treatment here. I think the problem is we don't really try to account for the formal arguments in the user SGPR count. A better fix would be to track that accurately

Pal uses a completely different approach where the front end does all the setup for the user sgprs and passes the info in via pal metadata. The backend can override/modify this, but for user data sgprs is generally doesn't. Moving the pal shaders to use the same model would be a fairly big piece of work, and would mean adding quite a lot of extra code to the backend that's specific to pal.

[arsenm]

In D129566#3645735, @dstuttard wrote:

In D129566#3645692, @arsenm wrote:

I don't see why PAL would get special treatment here. I think the problem is we don't really try to account for the formal arguments in the user SGPR count. A better fix would be to track that accurately

Pal uses a completely different approach where the front end does all the setup for the user sgprs and passes the info in via pal metadata. The backend can override/modify this, but for user data sgprs is generally doesn't. Moving the pal shaders to use the same model would be a fairly big piece of work, and would mean adding quite a lot of extra code to the backend that's specific to pal.

I'm not suggesting moving it into the same model, but the backend should figure out the user SGPR count given the formal argument list in the IR

[tpr]

I'm not suggesting moving it into the same model, but the backend should figure out the user SGPR count given the formal argument list in the IR

David is right -- we use a completely different model for PAL. The middle-end compiler LGC (part of LLPC) determines what user data sgprs there are, and what hardware-set sgprs (e.g. threadgroup_id0/1/2 in a CS) it wants to enable. It sets all the relevant bits and fields in the rsrc2 passed in IR metadata, and it adds the right number and type of inreg args to the function in the IR. For PAL, the backend would not modify those fields and bits in rsrc2 at all.

In the PAL model, the backend only sets fields that only it can know about, such as vgpr usage.

So this fix looks correct to me for PAL.

[dstuttard]

Additionally, not everything marked as inreg actually contributes to the NumUserSGPRs - so the knowledge about which arguments are and aren't would need to be added to the backend.

[nhaehnle]

To be honest, it looks like the entire function should only be called for ROCm-style compute. It shouldn't be used for either PAL or Mesa. Except maybe the Info.hasPrivateSegmentWaveByteOffset part? I'm not sure about that last one. But none of the hasWorkGroupFoo stuff should apply as far as I can tell.

And by the way, why is that a function of the OS? Shouldn't it be a function of the calling convention?

So to sum up: I agree with Tim and David that this code shouldn't be applied in the PAL/Mesa case (they both need their own fix for this) -- though keeping the assertion alive somewhere would be good. But it's not clear to me that this patch is the right way to do it.

It feels somehwat dubious that this entire method runs for an amdgpu_cs calling convention, but perhaps we don't need to change that, given the method already has an IsShader check. How about: apply the init16 bug workaround only if !IsShader, but keep the assertion unmodified?

[dstuttard]

I wasn't sure whether Mesa would also have issues - I was going to follow that up, so thanks for the info.
I'll update as you suggest to !IsShader

As for the assert - pal shaders won't return anything useful for Info.getNumPreloadedSGPRs() so the assert would always fail. I assume (perhaps wrongly) that Mesa would be the same?

[nhaehnle]

Thanks.

In D129566#3647912, @dstuttard wrote:

As for the assert - pal shaders won't return anything useful for Info.getNumPreloadedSGPRs() so the assert would always fail. I assume (perhaps wrongly) that Mesa would be the same?

Okay, no big deal. I thought it would return the number of SGPR arguments (since those are preloaded, it would kind of make sense).

[dstuttard]

Updated with review suggestions

[nhaehnle]

LGTM

[arsenm]

Testcase?

[dstuttard]

Added a test

I thought the best bet for this was something that showed the original patch
didn't alter user sgpr registers (and left RSRC2 as-is).

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

 }
 
 // Allocate special input registers that are initialized per-wave.
 void SITargetLowering::allocateSystemSGPRs(CCState &CCInfo,
                                            MachineFunction &MF,
                                            SIMachineFunctionInfo &Info,
                                            CallingConv::ID CallConv,
                                            bool IsShader) const {
-  if (Subtarget->hasUserSGPRInit16Bug()) {
+  if (Subtarget->hasUserSGPRInit16Bug() && !IsShader) {
+    // Note: user SGPRs are handled by the front-end for graphics shaders
     // Pad up the used user SGPRs with dead inputs.
     unsigned CurrentUserSGPRs = Info.getNumUserSGPRs();
 
     // Note we do not count the PrivateSegmentWaveByteOffset. We do not want to
     // rely on it to reach 16 since if we end up having no stack usage, it will
     // not really be added.
     unsigned NumRequiredSystemSGPRs = Info.hasWorkGroupIDX() +
                                       Info.hasWorkGroupIDY() +
if (IsShader) {
       }
     } else
       PrivateSegmentWaveByteOffsetReg = Info.addPrivateSegmentWaveByteOffset();
 
     MF.addLiveIn(PrivateSegmentWaveByteOffsetReg, &AMDGPU::SGPR_32RegClass);
     CCInfo.AllocateReg(PrivateSegmentWaveByteOffsetReg);
   }
 
-  assert(!Subtarget->hasUserSGPRInit16Bug() || Info.getNumPreloadedSGPRs() >= 16);
+  assert(!Subtarget->hasUserSGPRInit16Bug() || IsShader ||
+         Info.getNumPreloadedSGPRs() >= 16);
 }
 
 static void reservePrivateMemoryRegs(const TargetMachine &TM,
                                      MachineFunction &MF,
                                      const SIRegisterInfo &TRI,
                                      SIMachineFunctionInfo &Info) {
   // Now that we've figured out where the scratch register inputs are, see if
   // should reserve the arguments and use them directly.

llvm/test/CodeGen/AMDGPU/amdpal-usersgpr-init.ll

+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx1100 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -enable-var-scope %s
+
+; We want to make sure that RSRC2 is left untouched
+; GCN:       0x2e13 (COMPUTE_PGM_RSRC2): 0x78a
+define amdgpu_cs half @cs_amdpal(half %arg0, half inreg %arg1) {
+  %add = fadd half %arg0, 1.0
+  ret half %add
+}
+
+!amdgpu.pal.metadata.msgpack = !{!0}
+
+!0 = !{!"\82\B0amdpal.pipelines\91\89\A4.api\A6Vulkan\B0.hardware_stages\81\A3.cs\83\AB.sgpr_limith\AB.vgpr_limit\CD\01\00\AF.wavefront_size@\B7.internal_pipeline_hash\92\CF\E83\B3\C2\D1)\7FG\CF[\8A\DF\EE[\7FD,\AA.registers\8A\CD.\07\01\CD.\08\01\CD.\09\01\CD.\12\CE@,\00\00\CD.\13\CD\07\8A\CD.(\00\CD.*\CE\16\0B\22Y\CD.@\CE\10\00\00\00\CD.B\CE\10\00\00\06\CD.D\00\A8.shaders\81\A8.compute\82\B0.api_shader_hash\92\CF\D3s\A6\8D\C5x\84\D4\00\B1.hardware_mapping\91\A3.cs\B0.spill_threshold\CE\FF\FF\FF\FF\A5.type\A2Cs\B0.user_data_limit\01\AF.xgl_cache_info\82\B3.128_bit_cache_hash\92\CF\E5\A0\EB\F9}\C6\C1\13\CF\1A_\E7\F7\F2.mR\AD.llpc_version\A454.5\AEamdpal.version\92\02\03"}
 No newline at end of file