AMDGPU: Add a function attribute that shrinks buggy s_buffer opcodes on GFX9

[mareko]

Summary

This shouldn't be set for shaders running in an untrusted environment.

42952 affected shaders.
Code size in affected shaders: -12.64%

[arsenm]

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

165–166

I don't understand why the function attribute for a hardware bug. If this happens to help other targets not working around the bug we should add an optimization to do this somewhere.

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

We shouldn't be putting bug workarounds in an optimization pass. This should probably be part of the initial selection

[mareko]

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

165–166

Can you be more specific? I don't understand. Note that Mesa may set the function attribute differently for each shader, because it depends on whether the shader is trusted on untrusted.

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

What initial section are you talking about? Note that the placement of this pass is perfect, because the new code needs to run after SILoadStoreOptimizer.

[arsenm]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

One option would be to do this in the DAG and split the intrinsics before getting selected in the first place, or we could do this in AdjustInstrPostInstrSelection or in a new bug workaround pass.

Why does it need to be after SILoadStoreOptimizer? Is it just because it will try to merge and form these? If it's buggy it should just not do that in the first place.

[mareko]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

The intrinsic is translated into s_buffer_load_dword only. There are no intrinsics for the xN opcodes - this is actually the optimal situation because we have SILoadStoreOptimizer. SILoadStoreOptimizer merges s_buffer_load_dword into x2 and x4 - this is the only place that generates the xN opcodes. This new code needs to run after that to convert s_buffer_load into s_load for xN opcodes where N >= 2.

[dstuttard]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

I'm planning to add intrinsics that will allow multi-dword s_buffer_loads as this is considerably easier for our front-end. However, I guess the placement of this work-around means that this won't matter.

I agree that the work-around being after SILoadStoreOptimizer looks like the best place, but whether that should be as a separate bug workaround pass as Matt suggests is arguable - Matt, do you think it is just cleaner to have it pulled into a separate pass?

[nhaehnle]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

Shouldn't the SILoadStoreOptimizer just not generate the higher xN instructions on affected chips?

By the way, we may want to assume that buffers are aligned to e.g. 16 bytes and make this dependent on the alignment, e.g. if the low 4 bits of the saddr are known to be zero, we should still be able to use x4.

[mareko]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

Shouldn't the SILoadStoreOptimizer just not generate the higher xN instructions on affected chips

That's the current behavior, and it increases code size by 14.4%.

Loads are only guaranteed to be aligned to 4 bytes.

[nhaehnle]

Okay, I see it now, and I can live with it. Please still consider the alternative alignment approach - it is a bit more restrictive in what it can do, but it could be enabled unconditionally.

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

Loads are only guaranteed to be aligned to 4 bytes.

We could easily increase the required alignment on UBOs in radeonsi, let's say to 16 or even 64 bytes. I believe some other drivers set it to 256 bytes, which is the maximum allowed by the OpenGL spec. Then checking alignment for constant offsets becomes trivial, and for non-constant offsets we could still use computeKnownBits.

[mareko]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

The problem is, even when we set the required UBO alignment to 16, there is not guarantee that s_buffer_load_dwordx4 will be aligned to 16, because s_buffer_load_dwordx4 is constructed from multiple s_buffer_load_dword which load consecutive dwords, but it doesn't mean that the first one is aligned to 16. Or course, we could enforce the literal offset to be a multiple of 16, but I think that would decrease the number of opportunities for load opcode merging.

[nhaehnle]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

Yes, that's precisely what I meant. Yes, it provides a smaller number of opportunities, but it could be enabled unconditionally without a special flag. So it's still a useful trade-off.

[mareko]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

I'm a little concerned about app compatibility if we increase the alignment. 256 bytes was for ancient hw.

[arsenm]

lib/Target/AMDGPU/SIShrinkInstructions.cpp

310–312

This cannot be in this optimization pass. This must be done somewhere else

[mareko]

The workaround is not needed.

lib/Target/AMDGPU/AMDGPUSubtarget.h

public:
   bool hasMin3Max3_16() const {
     return getGeneration() >= GFX9;
   }
 
   bool hasMadMixInsts() const {
     return HasMadMixInsts;
   }
 
-  bool hasSBufferLoadStoreAtomicDwordxN() const {
+  bool hasBuggySBufferLoadStoreAtomicxN() const {
     // Only use the "x1" variants on GFX9 or don't use the buffer variants.
     // For x2 and higher variants, if the accessed region spans 2 VM pages and
     // the second page is unmapped, the hw hangs.
     // TODO: There is one future GFX9 chip that doesn't have this bug.
-    return getGeneration() != GFX9;
+    return getGeneration() == GFX9;
   }
 
   bool hasCARRY() const {
     return (getGeneration() >= EVERGREEN);
   }
 
   bool hasBORROW() const {
     return (getGeneration() >= EVERGREEN);

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

MachineBasicBlock::iterator SILoadStoreOptimizer::mergeBufferStorePair(
   CI.Paired->eraseFromParent();
   return Next;
 }
 
 // Scan through looking for adjacent LDS operations with constant offsets from
 // the same base register. We rely on the scheduler to do the hard work of
 // clustering nearby loads, and assume these are all adjacent.
 bool SILoadStoreOptimizer::optimizeBlock(MachineBasicBlock &MBB) {
+  const SIMachineFunctionInfo *MFI =
+    MBB.getParent()->getInfo<SIMachineFunctionInfo>();
   bool Modified = false;
 
   for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E;) {
     MachineInstr &MI = *I;
 
     // Don't combine if volatile.
     if (MI.hasOrderedMemoryRef()) {
       ++I;
if (Opc == AMDGPU::DS_READ_B32 || Opc == AMDGPU::DS_READ_B64 ||
         Modified = true;
         I = mergeWrite2Pair(CI);
       } else {
         ++I;
       }
 
       continue;
     }
-    if (STM->hasSBufferLoadStoreAtomicDwordxN() &&
+    if ((!STM->hasBuggySBufferLoadStoreAtomicxN() ||
+         MFI->shrinkBuggySBufferLoadStoreAtomicxN()) &&
         (Opc == AMDGPU::S_BUFFER_LOAD_DWORD_IMM ||
          Opc == AMDGPU::S_BUFFER_LOAD_DWORDX2_IMM)) {
       // EltSize is in units of the offset encoding.
       CI.InstClass = S_BUFFER_LOAD_IMM;
       CI.EltSize = AMDGPU::getSMRDEncodedOffset(*STM, 4);
       CI.IsX2 = Opc == AMDGPU::S_BUFFER_LOAD_DWORDX2_IMM;
       if (findMatchingInst(CI)) {
         Modified = true;

lib/Target/AMDGPU/SIMachineFunctionInfo.h

private:
   // Compute directly in sgpr[0:1]
   // Other shaders indirect 64-bits at sgpr[0:1]
   bool ImplicitBufferPtr : 1;
 
   // Pointer to where the ABI inserts special kernel arguments separate from the
   // user arguments. This is an offset from the KernargSegmentPtr.
   bool ImplicitArgPtr : 1;
 
+  // This converts s_buffer_xxx to s_xxx to allow xN loads on chips where
+  // the buffer opcodes are buggy, but at the cost of removing bounds checking
+  // that is provided by buffer opcodes.
+  //
+  // Constraint: Only the BASE_ADDRESS_HI field of WORD1 can be set, so that
+  // WORD0:WORD1 can trivially be used as an address.
+  bool ShrinkBuggySBufferLoadStoreAtomicxN : 1;
+
   // The hard-wired high half of the address of the global information table
   // for AMDPAL OS type. 0xffffffff represents no hard-wired high half, since
   // current hardware only allows a 16 bit value.
   unsigned GITPtrHigh;
 
   unsigned HighBitsOf32BitAddress;
 
   MCPhysReg getNextUserSGPR() const {
public:
   bool hasImplicitArgPtr() const {
     return ImplicitArgPtr;
   }
 
   bool hasImplicitBufferPtr() const {
     return ImplicitBufferPtr;
   }
 
+  unsigned shrinkBuggySBufferLoadStoreAtomicxN() const {
+    return ShrinkBuggySBufferLoadStoreAtomicxN;
+  }
+
   AMDGPUFunctionArgInfo &getArgInfo() {
     return ArgInfo;
   }
 
   const AMDGPUFunctionArgInfo &getArgInfo() const {
     return ArgInfo;
   }
 

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

: AMDGPUMachineFunction(MF),
     WorkGroupIDZ(false),
     WorkGroupInfo(false),
     PrivateSegmentWaveByteOffset(false),
     WorkItemIDX(false),
     WorkItemIDY(false),
     WorkItemIDZ(false),
     ImplicitBufferPtr(false),
     ImplicitArgPtr(false),
+    ShrinkBuggySBufferLoadStoreAtomicxN(false),
     GITPtrHigh(0xffffffff),
     HighBitsOf32BitAddress(0) {
   const SISubtarget &ST = MF.getSubtarget<SISubtarget>();
   const Function &F = MF.getFunction();
   FlatWorkGroupSizes = ST.getFlatWorkGroupSizes(F);
   WavesPerEU = ST.getWavesPerEU(F);
 
   if (!isEntryFunction()) {
SIMachineFunctionInfo::SIMachineFunctionInfo(const MachineFunction &MF)
 
   if (ST.hasFlatAddressSpace() && isEntryFunction() && IsCOV2) {
     // TODO: This could be refined a lot. The attribute is a poor way of
     // detecting calls that may require it before argument lowering.
     if (HasStackObjects || F.hasFnAttribute("amdgpu-flat-scratch"))
       FlatScratchInit = true;
   }
 
+  if (F.hasFnAttribute("amdgpu-shrink-buggy-sbuffer-opcodes"))
+    ShrinkBuggySBufferLoadStoreAtomicxN = true;
+
   Attribute A = F.getFnAttribute("amdgpu-git-ptr-high");
   StringRef S = A.getValueAsString();
   if (!S.empty())
     S.consumeInteger(0, GITPtrHigh);
 
   A = F.getFnAttribute("amdgpu-32bit-address-high-bits");
   S = A.getValueAsString();
   if (!S.empty())

lib/Target/AMDGPU/SIShrinkInstructions.cpp

 bool SIShrinkInstructions::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(MF.getFunction()))
     return false;
 
   MachineRegisterInfo &MRI = MF.getRegInfo();
   const SISubtarget &ST = MF.getSubtarget<SISubtarget>();
   const SIInstrInfo *TII = ST.getInstrInfo();
   const SIRegisterInfo &TRI = TII->getRegisterInfo();
+  const SIMachineFunctionInfo *MFI = MF.getInfo<SIMachineFunctionInfo>();
 
   std::vector<unsigned> I1Defs;
 
   for (MachineFunction::iterator BI = MF.begin(), BE = MF.end();
                                                   BI != BE; ++BI) {
 
     MachineBasicBlock &MBB = *BI;
     MachineBasicBlock::iterator I, Next;
     for (I = MBB.begin(); I != MBB.end(); I = Next) {
       Next = std::next(I);
       MachineInstr &MI = *I;
 
+      // Shrink buggy scalar buffer loads.
+      if (ST.hasBuggySBufferLoadStoreAtomicxN() &&
+          MFI->shrinkBuggySBufferLoadStoreAtomicxN() &&
+          TII->isSMRD(MI.getOpcode())) {
+        unsigned NewOpcode = 0;
+
+        // No other s_buffer opcodes can be generated by LLVM at the moment.
+        switch (MI.getOpcode()) {
+        case AMDGPU::S_BUFFER_LOAD_DWORDX2_IMM:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX2_IMM;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX4_IMM:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX4_IMM;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX8_IMM:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX8_IMM;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX16_IMM:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX16_IMM;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX2_SGPR:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX2_SGPR;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX4_SGPR:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX4_SGPR;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX8_SGPR:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX8_SGPR;
+          break;
+        case AMDGPU::S_BUFFER_LOAD_DWORDX16_SGPR:
+          NewOpcode = AMDGPU::S_LOAD_DWORDX16_SGPR;
+          break;
+        default:
+          continue;
+        }
+
+        unsigned SAddr = TII->buildExtractSubReg(MI, MRI, MI.getOperand(1),
+                                                 &AMDGPU::SReg_128RegClass,
+                                                 AMDGPU::sub0_sub1,
+                                                 &AMDGPU::SReg_64_XEXECRegClass);
+        MI.setDesc(TII->get(NewOpcode));
+        MI.getOperand(1).setReg(SAddr);
+        continue;
+      }
+
       if (MI.getOpcode() == AMDGPU::V_MOV_B32_e32) {
         // If this has a literal constant source that is the same as the
         // reversed bits of an inline immediate, replace with a bitreverse of
         // that constant. This saves 4 bytes in the common case of materializing
         // sign bits.
 
         // Test if we are after regalloc. We only want to do this after any
         // optimizations happen because this will confuse them.

test/CodeGen/AMDGPU/smrd.ll

main_body:
   %r4 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 16)
   %r5 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 28)
   %r6 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 32)
   call void @llvm.amdgcn.exp.f32(i32 0, i32 15, float %r1, float %r2, float %r3, float %r4, i1 true, i1 true) #0
   call void @llvm.amdgcn.exp.f32(i32 0, i32 15, float %r5, float %r6, float undef, float undef, i1 true, i1 true) #0
   ret void
 }
 
+; GCN-LABEL: {{^}}smrd_imm_merged_shrunk:
+; GCN-NEXT: %bb.
+; SICI-NEXT: s_buffer_load_dwordx4 s[{{[0-9]}}:{{[0-9]}}], s[0:3], 0x1
+; SICI-NEXT: s_buffer_load_dwordx2 s[{{[0-9]}}:{{[0-9]}}], s[0:3], 0x7
+; VI-NEXT: s_buffer_load_dwordx4 s[{{[0-9]}}:{{[0-9]}}], s[0:3], 0x4
+; VI-NEXT: s_buffer_load_dwordx2 s[{{[0-9]}}:{{[0-9]}}], s[0:3], 0x1c
+; GFX9-NEXT: s_load_dwordx4 s[{{[0-9]}}:{{[0-9]}}], s[0:1], 0x4
+; GFX9-NEXT: s_load_dwordx2 s[{{[0-9]}}:{{[0-9]}}], s[0:1], 0x1c
+define amdgpu_ps void @smrd_imm_merged_shrunk(<4 x i32> inreg %desc) #2 {
+main_body:
+  %r1 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 4)
+  %r2 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 8)
+  %r3 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 12)
+  %r4 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 16)
+  %r5 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 28)
+  %r6 = call float @llvm.SI.load.const.v4i32(<4 x i32> %desc, i32 32)
+  call void @llvm.amdgcn.exp.f32(i32 0, i32 15, float %r1, float %r2, float %r3, float %r4, i1 true, i1 true) #0
+  call void @llvm.amdgcn.exp.f32(i32 0, i32 15, float %r5, float %r6, float undef, float undef, i1 true, i1 true) #0
+  ret void
+}
+
 ; GCN-LABEL: {{^}}smrd_vgpr_merged:
 ; GCN-NEXT: %bb.
 ; GCN-NEXT: buffer_load_dwordx4 v[{{[0-9]}}:{{[0-9]}}], v0, s[0:3], 0 offen offset:4
 ; GCN-NEXT: buffer_load_dwordx2 v[{{[0-9]}}:{{[0-9]}}], v0, s[0:3], 0 offen offset:28
 define amdgpu_ps void @smrd_vgpr_merged(<4 x i32> inreg %desc, i32 %a) #0 {
 main_body:
   %a1 = add i32 %a, 4
   %a2 = add i32 %a, 8
main_body:
   ret void
 }
 
 declare void @llvm.amdgcn.exp.f32(i32, i32, float, float, float, float, i1, i1) #0
 declare float @llvm.SI.load.const.v4i32(<4 x i32>, i32) #1
 
 attributes #0 = { nounwind }
 attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind "amdgpu-shrink-buggy-sbuffer-opcodes" }