[amdgpu] Change LDS lowering default to hybrid

[JonChesterfield]

Summary

Postponed from D139433 until the bug fixed by D139874 could be resolved.

[JonChesterfield]

Internal CI number 792330 running at present.

[arsenm]

I would have expected a test update?

[arsenm]

I also wouldn’t call it a regalloc bug and point to to commit in the message

[JonChesterfield]

• Change to hybrid. Reintroduces code block deleted by git misfire

[JonChesterfield]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

1331–1343

This is latent because the module lds variable is always lowered to zero and the path through codegen coincidentally picks zero for it. It breaks on the "kernel" lowering strategy because that can reference variables that aren't at zero directly, and breaks on the "hybrid" lowering when it happens to pick the "kernel" strategy for a variable allocated by a kernel which does not use the "module" scope variable.

Still, relying on unspecified -> zero is bad. Planning to add a test case for this behaviour change (it won't show up in codegen that picks zero anyway, but should do for codegen where zero was not the right answer)

[JonChesterfield]

• update test, no longer emitting traps

[JonChesterfield]

Checked in the new test case (codegen for hybrid path, previously only checking IR) before this patch in order to highlight the s_trap -> working change. Will commit this as is to catch up with rocm and then change to absolute_symbols (as in D144221), patching trunk first so that rocm picks up the change without manual intervention.

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

 
 SDValue AMDGPUTargetLowering::LowerGlobalAddress(AMDGPUMachineFunction* MFI,
                                                  SDValue Op,
                                                  SelectionDAG &DAG) const {
 
   const DataLayout &DL = DAG.getDataLayout();
   GlobalAddressSDNode *G = cast<GlobalAddressSDNode>(Op);
   const GlobalValue *GV = G->getGlobal();
 
+  if (G->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS) {
+    if (!MFI->isModuleEntryFunction()) {
+      if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV)) {
+        if (AMDGPUMachineFunction::isKnownAddressLDSGlobal(*GVar)) {
+          unsigned Offset =
+              AMDGPUMachineFunction::calculateKnownAddressOfLDSGlobal(*GVar);
+          return DAG.getConstant(Offset, SDLoc(Op), Op.getValueType());
+        }
+      }
+    }
+  }
+
   if (G->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS ||
       G->getAddressSpace() == AMDGPUAS::REGION_ADDRESS) {
     if (!MFI->isModuleEntryFunction() &&
         !GV->getName().equals("llvm.amdgcn.module.lds")) {
       SDLoc DL(Op);
       const Function &Fn = DAG.getMachineFunction().getFunction();
       DiagnosticInfoUnsupported BadLDSDecl(
         Fn, "local memory global used by non-kernel function",

llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp

cl::opt<bool> SuperAlignLDSGlobals(
     "amdgpu-super-align-lds-globals",
     cl::desc("Increase alignment of LDS if it is not on align boundary"),
     cl::init(true), cl::Hidden);
 
 enum class LoweringKind { module, table, kernel, hybrid };
 cl::opt<LoweringKind> LoweringKindLoc(
     "amdgpu-lower-module-lds-strategy",
     cl::desc("Specify lowering strategy for function LDS access:"), cl::Hidden,
-    cl::init(LoweringKind::module),
+    cl::init(LoweringKind::hybrid),
     cl::values(
         clEnumValN(LoweringKind::table, "table", "Lower via table lookup"),
         clEnumValN(LoweringKind::module, "module", "Lower via module struct"),
         clEnumValN(
             LoweringKind::kernel, "kernel",
             "Lower variables reachable from one kernel, otherwise abort"),
         clEnumValN(LoweringKind::hybrid, "hybrid",
                    "Lower via mixture of above strategies")));

llvm/test/CodeGen/AMDGPU/lower-module-lds-via-hybrid.ll

 ; OPT-NEXT:    %ld = load float, ptr addrspace(3) @llvm.amdgcn.kernel.k01.lds, align 4
 ; OPT-NEXT:    %mul = fmul float %ld, 2.000000e+00
 ; OPT-NEXT:    store float %mul, ptr addrspace(3) @llvm.amdgcn.kernel.k01.lds, align 4
 ; OPT-NEXT:    ret void
 ;
 ; GCN-LABEL: f0:
 ; GCN:       ; %bb.0:
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GCN-NEXT:    s_mov_b64 s[0:1], s[6:7]
-; GCN-NEXT:    s_trap 2
+; GCN-NEXT:    v_mov_b32_e32 v0, 0
 ; GCN-NEXT:    s_mov_b32 m0, -1
-; GCN-NEXT:    ds_read_b32 v0, v0
+; GCN-NEXT:    ds_read_b32 v1, v0 offset:4
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)
-; GCN-NEXT:    v_add_f32_e32 v0, v0, v0
-; GCN-NEXT:    ds_write_b32 v0, v0
+; GCN-NEXT:    v_add_f32_e32 v1, v1, v1
+; GCN-NEXT:    ds_write_b32 v0, v1 offset:4
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
   %ld = load float, ptr addrspace(3) @v0
   %mul = fmul float %ld, 2.
   store float %mul, ptr  addrspace(3) @v0
   ret void
 }
 

 ; OPT-NEXT:    %ld = load i8, ptr addrspace(3) getelementptr inbounds (%llvm.amdgcn.kernel.k23.lds.t, ptr addrspace(3) @llvm.amdgcn.kernel.k23.lds, i32 0, i32 1), align 8
 ; OPT-NEXT:    %mul = mul i8 %ld, 5
 ; OPT-NEXT:    store i8 %mul, ptr addrspace(3) getelementptr inbounds (%llvm.amdgcn.kernel.k23.lds.t, ptr addrspace(3) @llvm.amdgcn.kernel.k23.lds, i32 0, i32 1), align 8
 ; OPT-NEXT:    ret void
 ;
 ; GCN-LABEL: f3:
 ; GCN:       ; %bb.0:
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GCN-NEXT:    s_mov_b64 s[0:1], s[6:7]
-; GCN-NEXT:    s_trap 2
+; GCN-NEXT:    v_mov_b32_e32 v0, 0
 ; GCN-NEXT:    s_mov_b32 m0, -1
-; GCN-NEXT:    ds_read_u8 v0, v0
+; GCN-NEXT:    ds_read_u8 v1, v0 offset:8
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)
-; GCN-NEXT:    v_mul_lo_u32 v0, v0, 5
-; GCN-NEXT:    ds_write_b8 v0, v0
+; GCN-NEXT:    v_mul_lo_u32 v1, v1, 5
+; GCN-NEXT:    ds_write_b8 v0, v1 offset:8
 ; GCN-NEXT:    s_waitcnt lgkmcnt(0)
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
   %ld = load i8, ptr addrspace(3) @v3
   %mul = mul i8 %ld, 5
   store i8 %mul, ptr  addrspace(3) @v3
   ret void
 }