AMDGPU: Correct const_index_stride for wave 32 for PAL ABI

[dstuttard]

Summary

Since there is a single scratch resource descriptor for all shaders, if there is
a wave32 and a wave64 shader (for instance for VsFs pairs)
then the const_index_stride will be incorrect for wave32 shaders.

[foad]

Needs a test case.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

551–553

The comment could be a bit more helpful. As I understand it: index_stride is a 2-bit field in bits 118:117 of the descriptor, so bits 22:21 of this word. (Is this the same for all architectures?) The value coming in might be either 0b10 (stride=32) or 0b11 (stride=64) so we force it to 0b10 by clearing bit 21.

[foad]

"API" should be "ABI" in the title.

[dstuttard]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

551–553

Yes - the driver will always set this up assuming wave64. Just setting the bits to the wave32 option covers that case (as well as the supposedly non-occurring case where the value is already correct for wave32).

How about:
The driver will always set the SRD for wave 64. If the shader is actually wave32 we have to modify the const_index_stride field of the descriptor (bits 22:21) to b10 (stride=32). The reason the driver does this is that there can be cases where it presents 2 shaders with different wave size (e.g. VsFs).

[dstuttard]

API -> ABI

[dstuttard]

In D101830#2736079, @foad wrote:

"API" should be "ABI" in the title.

Hmm, changed the commit, but the review title remains unchanged.

[arsenm]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

557

Can you use S_BITSET0_B32 instead?

[foad]

In D101830#2736090, @dstuttard wrote:

In D101830#2736079, @foad wrote:

"API" should be "ABI" in the title.

Hmm, changed the commit, but the review title remains unchanged.

Yeah, arc never seems to update that field once the review has been created. You can do it manually from the web interface.

[foad]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

551–553

Sounds OK, though I think it would be better to mention the value 0b11 too, otherwise it's not obvious why just setting a single bit to 0 has the desired effect.

Also I've never seen binary literals with just a "b" prefix which is why I used "0b".

[dstuttard]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

557

Could do - the s_and gets transformed somewhere to s_bitset anyway.

[dstuttard]

Change to bitset0, extend comment and update a test

[RKSimon]

@dstuttard I think this is causing errors on EXPENSIVE_CHECKS builds - please can you take a look?

$ ":" "RUN: at line 3"
$ "e:\llvm\ninja\bin\opt.exe" "-S" "-mtriple=amdgcn-amd-amdpal" "-amdgpu-annotate-kernel-features" "E:\llvm\llvm-project\llvm\test\CodeGen\AMDGPU\pal-simple-indirect-call.ll"
$ "e:\llvm\ninja\bin\filecheck.exe" "-check-prefix=GCN" "E:\llvm\llvm-project\llvm\test\CodeGen\AMDGPU\pal-simple-indirect-call.ll"
$ ":" "RUN: at line 6"
$ "e:\llvm\ninja\bin\llc.exe" "-mtriple=amdgcn-amd-amdpal" "-mcpu=gfx900"
$ "e:\llvm\ninja\bin\filecheck.exe" "-check-prefixes=GFX9" "E:\llvm\llvm-project\llvm\test\CodeGen\AMDGPU\pal-simple-indirect-call.ll"
$ ":" "RUN: at line 7"
$ "e:\llvm\ninja\bin\llc.exe" "-mtriple=amdgcn-amd-amdpal" "-mcpu=gfx1010"
# command stderr:

# After Prologue/Epilogue Insertion & Frame Finalization
# Machine code for function test_simple_indirect_call: NoPHIs, TracksLiveness, NoVRegs, TiedOpsRewritten
Function Live Ins: $sgpr0, $sgpr0

bb.0 (%ir-block.0):
  liveins: $sgpr0, $sgpr0
  $sgpr32 = S_MOV_B32 0
  $sgpr36_sgpr37 = S_GETPC_B64
  $sgpr36 = S_MOV_B32 $sgpr0
  $sgpr36_sgpr37_sgpr38_sgpr39 = S_LOAD_DWORDX4_IMM $sgpr36_sgpr37, 16, 0, implicit-def $sgpr36_sgpr37_sgpr38_sgpr39 :: (dereferenceable invariant load 16, align 4, addrspace 4)
  $sgpr39 = S_BITSET0_B32 21, implicit-def $sgpr39
  $sgpr36 = S_ADD_U32 $sgpr36, $sgpr0, implicit-def $scc, implicit-def $sgpr36_sgpr37_sgpr38_sgpr39
  $sgpr37 = S_ADDC_U32 $sgpr37, 0, implicit-def $scc, implicit $scc, implicit-def $sgpr36_sgpr37_sgpr38_sgpr39
  renamable $sgpr4_sgpr5 = S_GETPC_B64
  $sgpr0_sgpr1_sgpr2_sgpr3 = COPY $sgpr36_sgpr37_sgpr38_sgpr39
  dead $sgpr30_sgpr31 = SI_CALL killed renamable $sgpr4_sgpr5, 0, <regmask $sgpr32 $sgpr33 $sgpr34 $sgpr35 $sgpr36 $sgpr37 $sgpr38 $sgpr39 $sgpr40 $sgpr41 $sgpr42 $sgpr43 $sgpr44 $sgpr45 $sgpr46 $sgpr47 $sgpr48 $sgpr49 $sgpr50 $sgpr51 $sgpr52 $sgpr53 $sgpr54 $sgpr55 $sgpr56 $sgpr57 $sgpr58 $sgpr59 $sgpr60 $sgpr61 $sgpr62 $sgpr63 $sgpr64 and 1047 more...>, implicit $sgpr0_sgpr1_sgpr2_sgpr3
  S_ENDPGM 0

# End machine code for function test_simple_indirect_call.

*** Bad machine code: Explicit operand marked as def ***
- function:    test_simple_indirect_call
- basic block: %bb.0  (0x195b4d11340)
- instruction: $sgpr39 = S_BITSET0_B32 21, implicit-def $sgpr39
- operand 2:   implicit-def $sgpr39

*** Bad machine code: Explicit operand marked as implicit ***
- function:    test_simple_indirect_call
- basic block: %bb.0  (0x195b4d11340)
- instruction: $sgpr39 = S_BITSET0_B32 21, implicit-def $sgpr39
- operand 2:   implicit-def $sgpr39

*** Bad machine code: Operand should be tied ***
- function:    test_simple_indirect_call
- basic block: %bb.0  (0x195b4d11340)
- instruction: $sgpr39 = S_BITSET0_B32 21, implicit-def $sgpr39
- operand 2:   implicit-def $sgpr39
LLVM ERROR: Found 3 machine code errors.

[dstuttard]

Yes, thanks. Already reverted.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

void SIFrameLowering::emitEntryFunctionScratchRsrcRegSetup(
   const SIRegisterInfo *TRI = &TII->getRegisterInfo();
   const SIMachineFunctionInfo *MFI = MF.getInfo<SIMachineFunctionInfo>();
   const Function &Fn = MF.getFunction();
 
   if (ST.isAmdPalOS()) {
     // The pointer to the GIT is formed from the offset passed in and either
     // the amdgpu-git-ptr-high function attribute or the top part of the PC
     Register Rsrc01 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0_sub1);
+    Register Rsrc03 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub3);
 
     buildGitPtr(MBB, I, DL, TII, Rsrc01);
 
     // We now have the GIT ptr - now get the scratch descriptor from the entry
     // at offset 0 (or offset 16 for a compute shader).
     MachinePointerInfo PtrInfo(AMDGPUAS::CONSTANT_ADDRESS);
     const MCInstrDesc &LoadDwordX4 = TII->get(AMDGPU::S_LOAD_DWORDX4_IMM);
     auto MMO = MF.getMachineMemOperand(PtrInfo,
                                        MachineMemOperand::MOLoad |
                                            MachineMemOperand::MOInvariant |
                                            MachineMemOperand::MODereferenceable,
                                        16, Align(4));
     unsigned Offset = Fn.getCallingConv() == CallingConv::AMDGPU_CS ? 16 : 0;
     const GCNSubtarget &Subtarget = MF.getSubtarget<GCNSubtarget>();
     unsigned EncodedOffset = AMDGPU::convertSMRDOffsetUnits(Subtarget, Offset);
     BuildMI(MBB, I, DL, LoadDwordX4, ScratchRsrcReg)
       .addReg(Rsrc01)
       .addImm(EncodedOffset) // offset
       .addImm(0) // cpol
       .addReg(ScratchRsrcReg, RegState::ImplicitDefine)
       .addMemOperand(MMO);
+
+    // The driver will always set the SRD for wave 64 (bits 118:117 of
+    // descriptor / bits 22:21 of third sub-reg will be 0b11)
+    // If the shader is actually wave32 we have to modify the const_index_stride
+    // field of the descriptor 3rd sub-reg (bits 22:21) to 0b10 (stride=32). The
+    // reason the driver does this is that there can be cases where it presents
+    // 2 shaders with different wave size (e.g. VsFs).
+    // TODO: convert to using SCRATCH instructions or multiple SRD buffers
+    if (ST.isWave32()) {
+      const MCInstrDesc &SBitsetB32 = TII->get(AMDGPU::S_BITSET0_B32);
+      BuildMI(MBB, I, DL, SBitsetB32, Rsrc03)
+          .addImm(21)
+          .addReg(Rsrc03, RegState::ImplicitDefine);
+    }
   } else if (ST.isMesaGfxShader(Fn) || !PreloadedScratchRsrcReg) {
     assert(!ST.isAmdHsaOrMesa(Fn));
     const MCInstrDesc &SMovB32 = TII->get(AMDGPU::S_MOV_B32);
 
     Register Rsrc2 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub2);
     Register Rsrc3 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub3);
 
     // Use relocations to get the pointer, and setup the other bits manually.

llvm/test/CodeGen/AMDGPU/pal-simple-indirect-call.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; Check that no attributes are added to graphics functions
 ; RUN: opt -S -mtriple=amdgcn-amd-amdpal -amdgpu-annotate-kernel-features  %s | FileCheck -check-prefix=GCN %s
 
 ; Check that it doesn't crash
-; RUN: llc -mtriple=amdgcn-amd-amdpal -mcpu=gfx1010 < %s | FileCheck -check-prefix=GFX9 %s
+; RUN: llc -mtriple=amdgcn-amd-amdpal -mcpu=gfx900 < %s | FileCheck -check-prefixes=GFX9 %s
+; RUN: llc -mtriple=amdgcn-amd-amdpal -mcpu=gfx1010 < %s | FileCheck -check-prefixes=GFX10 %s
 
 target datalayout = "A5"
 
 define amdgpu_cs void @test_simple_indirect_call() {
 ; GCN-LABEL: define amdgpu_cs void @test_simple_indirect_call() {
 ; GFX9-LABEL: test_simple_indirect_call:
 ; GFX9:       ; %bb.0:
 ; GFX9-NEXT:    s_getpc_b64 s[36:37]
 ; GFX9-NEXT:    s_mov_b32 s36, s0
-; GFX9-NEXT:    s_getpc_b64 s[4:5]
 ; GFX9-NEXT:    s_load_dwordx4 s[36:39], s[36:37], 0x10
+; GFX9-NEXT:    s_getpc_b64 s[4:5]
 ; GFX9-NEXT:    s_mov_b32 s32, 0
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-NEXT:    s_add_u32 s36, s36, s0
 ; GFX9-NEXT:    s_addc_u32 s37, s37, 0
 ; GFX9-NEXT:    s_mov_b64 s[0:1], s[36:37]
 ; GFX9-NEXT:    s_mov_b64 s[2:3], s[38:39]
 ; GFX9-NEXT:    s_swappc_b64 s[30:31], s[4:5]
 ; GFX9-NEXT:    s_endpgm
+;
+; GFX10-LABEL: test_simple_indirect_call:
+; GFX10:       ; %bb.0:
+; GFX10-NEXT:    s_getpc_b64 s[36:37]
+; GFX10-NEXT:    s_mov_b32 s36, s0
+; GFX10-NEXT:    s_getpc_b64 s[4:5]
+; GFX10-NEXT:    s_load_dwordx4 s[36:39], s[36:37], 0x10
+; GFX10-NEXT:    s_waitcnt lgkmcnt(0)
+; GFX10-NEXT:    s_bitset0_b32 s39, 21
+; GFX10-NEXT:    s_mov_b32 s32, 0
+; GFX10-NEXT:    s_add_u32 s36, s36, s0
+; GFX10-NEXT:    s_addc_u32 s37, s37, 0
+; GFX10-NEXT:    s_mov_b64 s[0:1], s[36:37]
+; GFX10-NEXT:    s_mov_b64 s[2:3], s[38:39]
+; GFX10-NEXT:    s_swappc_b64 s[30:31], s[4:5]
+; GFX10-NEXT:    s_endpgm
+
 
   %pc = call i64 @llvm.amdgcn.s.getpc()
   %fun = inttoptr i64 %pc to void()*
   call amdgpu_gfx void %fun()
   ret void
 }
 
 ; Function Attrs: nounwind readnone speculatable willreturn
 declare i64 @llvm.amdgcn.s.getpc() #0
 
 attributes #0 = { nounwind readnone speculatable willreturn }