[AMDGPU] Support emitting GOT relocations for function calls

[scott.linder]

Summary

Currently we make this decision based on addrspace, with the rationale being that Private (and so Generic/Flat) can't meaningfully support relocations. Until recently functions could not have a non-default (i.e. non-Generic/Flat) addrspace at all, meaning we never emitted GOT relocations for functions.

The approach taken in this patch is to explicitly support GOT for functions, regardless of addrspace. We might instead consider whether addrspace is the correct way to make the decision in the first place. In the long term we may also wish to change the default addrspace of our functions.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3488–3492

Looking for a specific load opcode looks concerning. Where is this coming from? I would think we need a pseudo at this point

[scott.linder]

lib/Target/AMDGPU/SIISelLowering.cpp

3488–3492

SITargetLowering::LowerGlobalAddress adds the load after creating the SI_PC_ADD_REL_OFFSET pseudo. Would it be better to add a new pseudo for just the load, or create a pseudo to include the existing PC relative calculation and the load, i.e. SI_LOAD_PC_ADD_REL_OFFSET? I don't think the load needs to be bundled to enforce an ordering or anything.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3488–3492

I'm worried about selecting a different opcode, and the possibility of some transform somewhere obscuring the address.

I think I looked at this before, and other targets use different call opcodes based on the address type. I think I did it this way because unlike other targets, we don't embed the call address in the actual call instruction. I think using the second pseudo would be closer to this, though I still don't like relying on getVRegDef here.

[scott.linder]

lib/Target/AMDGPU/SIISelLowering.cpp

3488–3492

I started to implement another pseudo, but as you mention getVRegDef doesn't seem right here. Is there a reason why the Global isn't carried through all of ISel, rather than being "recovered" later?

[scott.linder]

I've tried to implement carrying a copy of the function global through the DAG from the beginning, rather than recovering it after legalizing the GlobalAddress. I don't know if this is a reasonable approach, but I figured I would post something and see what you think.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3496

What happened to TCRETURN? I'm pretty sure we need this

test/CodeGen/AMDGPU/byval-frame-setup.ll

17 ↗

(On Diff #184603)

Can you commit the test changes adding hidden first?

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3998–4001

Can you add a comment here

[scott.linder]

Address feedback

lib/Target/AMDGPU/SIISelLowering.cpp

3496

After removing the need to insert the original global here there was nothing to do for TCRETURN in the custom inserter, so I just deleted the _ISEL variant and used tablegen to legalize ISD::TC_RETURN directly to SI_TCRETURN.

[arsenm]

test/CodeGen/AMDGPU/function-call-relocs.ll

48

Can you add a case with a tail call that needs the GOT just to be sure?

[scott.linder]

Add test for tail call through GOT

[arsenm]

LGTM

lib/Target/AMDGPU/AMDGPUInstrInfo.td

 def AMDGPUIfBreakOp : SDTypeProfile<1, 2,
   [SDTCisVT<0, i64>, SDTCisVT<1, i1>, SDTCisVT<2, i64>]
 >;
 
 def AMDGPUAddeSubeOp : SDTypeProfile<2, 3,
   [SDTCisSameAs<0, 2>, SDTCisSameAs<0, 3>, SDTCisVT<0, i32>, SDTCisVT<1, i1>, SDTCisVT<4, i1>]
 >;
 
-def SDT_AMDGPUTCRET : SDTypeProfile<0, 2, [SDTCisPtrTy<0>]>;
-
 //===----------------------------------------------------------------------===//
 // AMDGPU DAG Nodes
 //
 
 def AMDGPUif : SDNode<"AMDGPUISD::IF", AMDGPUIfOp, [SDNPHasChain]>;
 def AMDGPUelse : SDNode<"AMDGPUISD::ELSE", AMDGPUElseOp, [SDNPHasChain]>;
 def AMDGPUloop : SDNode<"AMDGPUISD::LOOP", AMDGPULoopOp, [SDNPHasChain]>;
 
 def callseq_start : SDNode<"ISD::CALLSEQ_START",
   SDCallSeqStart<[ SDTCisVT<0, i32>, SDTCisVT<1, i32> ]>,
   [SDNPHasChain, SDNPOutGlue]
 >;
 
 def callseq_end : SDNode<"ISD::CALLSEQ_END",
  SDCallSeqEnd<[ SDTCisVT<0, i32>, SDTCisVT<1, i32> ]>,
   [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue]
 >;
 
 def AMDGPUcall : SDNode<"AMDGPUISD::CALL",
   SDTypeProfile<0, -1, [SDTCisPtrTy<0>]>,
   [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
   SDNPVariadic]
 >;
 
-def AMDGPUtc_return: SDNode<"AMDGPUISD::TC_RETURN", SDT_AMDGPUTCRET,
+def AMDGPUtc_return: SDNode<"AMDGPUISD::TC_RETURN",
+  SDTypeProfile<0, 3, [SDTCisPtrTy<0>]>,
   [SDNPHasChain, SDNPOptInGlue, SDNPVariadic]
 >;
 
 def AMDGPUtrap : SDNode<"AMDGPUISD::TRAP",
   SDTypeProfile<0, -1, [SDTCisVT<0, i16>]>,
     [SDNPHasChain, SDNPVariadic, SDNPSideEffect, SDNPInGlue]
 >;
 

lib/Target/AMDGPU/SIISelLowering.cpp

Chain = DAG.getCALLSEQ_END(Chain,
                                DAG.getTargetConstant(0, DL, MVT::i32),
                                InFlag, DL);
     InFlag = Chain.getValue(1);
   }
 
   std::vector<SDValue> Ops;
   Ops.push_back(Chain);
   Ops.push_back(Callee);
+  // Add a redundant copy of the callee global which will not be legalized, as
+  // we need direct access to the callee later.
+  GlobalAddressSDNode *GSD = cast<GlobalAddressSDNode>(Callee);
+  const GlobalValue *GV = GSD->getGlobal();
+  Ops.push_back(DAG.getTargetGlobalAddress(GV, DL, MVT::i64));
 
   if (IsTailCall) {
     // Each tail call may have to adjust the stack by a different amount, so
     // this information must travel along with the operation for eventual
     // consumption by emitEpilogue.
     Ops.push_back(DAG.getTargetConstant(FPDiff, DL, MVT::i32));
 
     Ops.push_back(PhysReturnAddrReg);
case AMDGPU::ADJCALLSTACKDOWN: {
     // Add an implicit use of the frame offset reg to prevent the restore copy
     // inserted after the call from being reorderd after stack operations in the
     // the caller's frame.
     MIB.addReg(Info->getStackPtrOffsetReg(), RegState::ImplicitDefine)
         .addReg(Info->getStackPtrOffsetReg(), RegState::Implicit)
         .addReg(Info->getFrameOffsetReg(), RegState::Implicit);
     return BB;
   }
-  case AMDGPU::SI_CALL_ISEL:
-  case AMDGPU::SI_TCRETURN_ISEL: {
+  case AMDGPU::SI_CALL_ISEL: {
     const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
     const DebugLoc &DL = MI.getDebugLoc();
-    unsigned ReturnAddrReg = TII->getRegisterInfo().getReturnAddressReg(*MF);
 
-    MachineRegisterInfo &MRI = MF->getRegInfo();
-    unsigned GlobalAddrReg = MI.getOperand(0).getReg();
-    MachineInstr *PCRel = MRI.getVRegDef(GlobalAddrReg);
-    assert(PCRel->getOpcode() == AMDGPU::SI_PC_ADD_REL_OFFSET);
-
-    const GlobalValue *G = PCRel->getOperand(1).getGlobal();
+    unsigned ReturnAddrReg = TII->getRegisterInfo().getReturnAddressReg(*MF);
 
     MachineInstrBuilder MIB;
-    if (MI.getOpcode() == AMDGPU::SI_CALL_ISEL) {
-      MIB = BuildMI(*BB, MI, DL, TII->get(AMDGPU::SI_CALL), ReturnAddrReg)
-        .add(MI.getOperand(0))
-        .addGlobalAddress(G);
-    } else {
-      MIB = BuildMI(*BB, MI, DL, TII->get(AMDGPU::SI_TCRETURN))
-        .add(MI.getOperand(0))
-        .addGlobalAddress(G);
-
-      // There is an additional imm operand for tcreturn, but it should be in the
-      // right place already.
-    }
+    MIB = BuildMI(*BB, MI, DL, TII->get(AMDGPU::SI_CALL), ReturnAddrReg);
 
-    for (unsigned I = 1, E = MI.getNumOperands(); I != E; ++I)
+    for (unsigned I = 0, E = MI.getNumOperands(); I != E; ++I)
       MIB.add(MI.getOperand(I));
 
     MIB.cloneMemRefs(MI);
     MI.eraseFromParent();
     return BB;
   }
   default:
     return AMDGPUTargetLowering::EmitInstrWithCustomInserter(MI, BB);
   }

 bool SITargetLowering::shouldEmitFixup(const GlobalValue *GV) const {
   const Triple &TT = getTargetMachine().getTargetTriple();
   return (GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS ||
           GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS_32BIT) &&
          AMDGPU::shouldEmitConstantsToTextSection(TT);
 }
 
 bool SITargetLowering::shouldEmitGOTReloc(const GlobalValue *GV) const {
-  return (GV->getType()->getAddressSpace() == AMDGPUAS::GLOBAL_ADDRESS ||
+  // FIXME: Either avoid relying on address space here or change the default
+  // address space for functions to avoid the explicit check.
+  return (GV->getValueType()->isFunctionTy() ||
+          GV->getType()->getAddressSpace() == AMDGPUAS::GLOBAL_ADDRESS ||
           GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS ||
           GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS_32BIT) &&
          !shouldEmitFixup(GV) &&
          !getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV);
 }
 
 bool SITargetLowering::shouldEmitPCReloc(const GlobalValue *GV) const {
   return !shouldEmitFixup(GV) && !shouldEmitGOTReloc(GV);

lib/Target/AMDGPU/SIInstructions.td

def SI_RETURN : SPseudoInstSI <
   let SchedRW = [WriteBranch];
 }
 
 // Return for returning function calls without output register.
 //
 // This version is only needed so we can fill in the output regiter in
 // the custom inserter.
 def SI_CALL_ISEL : SPseudoInstSI <
-  (outs), (ins SSrc_b64:$src0), [(AMDGPUcall i64:$src0)]> {
+  (outs), (ins SSrc_b64:$src0, unknown:$callee),
+  [(AMDGPUcall i64:$src0, tglobaladdr:$callee)]> {
   let Size = 4;
   let isCall = 1;
   let SchedRW = [WriteBranch];
   let usesCustomInserter = 1;
 }
 
 // Wrapper around s_swappc_b64 with extra $callee parameter to track
 // the called function after regalloc.
 def SI_CALL : SPseudoInstSI <
   (outs SReg_64:$dst), (ins SSrc_b64:$src0, unknown:$callee)> {
   let Size = 4;
   let isCall = 1;
   let UseNamedOperandTable = 1;
   let SchedRW = [WriteBranch];
 }
 
 // Tail call handling pseudo
-def SI_TCRETURN_ISEL : SPseudoInstSI<(outs),
-  (ins SSrc_b64:$src0, i32imm:$fpdiff),
-  [(AMDGPUtc_return i64:$src0, i32:$fpdiff)]> {
-  let isCall = 1;
-  let isTerminator = 1;
-  let isReturn = 1;
-  let isBarrier = 1;
-  let SchedRW = [WriteBranch];
-  let usesCustomInserter = 1;
-}
-
-def SI_TCRETURN : SPseudoInstSI <
-  (outs),
-  (ins SSrc_b64:$src0, unknown:$callee, i32imm:$fpdiff)> {
+def SI_TCRETURN : SPseudoInstSI <(outs),
+  (ins SSrc_b64:$src0, unknown:$callee, i32imm:$fpdiff),
+  [(AMDGPUtc_return i64:$src0, tglobaladdr:$callee, i32:$fpdiff)]> {
   let Size = 4;
   let isCall = 1;
   let isTerminator = 1;
   let isReturn = 1;
   let isBarrier = 1;
   let UseNamedOperandTable = 1;
   let SchedRW = [WriteBranch];
 }

test/CodeGen/AMDGPU/function-call-relocs.ll

+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=fiji < %s | FileCheck %s
+
+define void @func(i32 addrspace(1)* %out) #0 {
+  store i32 1, i32 addrspace(1)* %out
+  ret void
+}
+
+define protected void @protected_func(i32 addrspace(1)* %out) #0 {
+  store i32 1, i32 addrspace(1)* %out
+  ret void
+}
+
+define hidden void @hidden_func(i32 addrspace(1)* %out) #0 {
+  store i32 1, i32 addrspace(1)* %out
+  ret void
+}
+
+; CHECK-LABEL: call_func:
+; CHECK: s_getpc_b64 s{{\[}}[[PC_LO:[0-9]+]]:[[PC_HI:[0-9]+]]{{\]}}
+; CHECK: s_add_u32 s[[GOT_ADDR_LO:[0-9]+]], s[[PC_LO]], func@gotpcrel32@lo+4
+; CHECK: s_addc_u32 s[[GOT_ADDR_HI:[0-9]+]], s[[PC_HI]], func@gotpcrel32@hi+4
+; CHECK: s_load_dwordx2 s{{\[}}[[ADDR_LO:[0-9]+]]:[[ADDR_HI:[0-9]+]]{{\]}}, s{{\[}}[[GOT_ADDR_LO]]:[[GOT_ADDR_HI]]{{\]}}, 0x0
+; CHECK: s_swappc_b64 s{{\[}}{{[0-9]+:[0-9]+}}{{\]}}, s{{\[}}[[ADDR_LO]]:[[ADDR_HI]]{{\]}}
+define amdgpu_kernel void @call_func(i32 addrspace(1)* %out) {
+  call void @func(i32 addrspace(1)* %out)
+  ret void
+}
+
+; CHECK-LABEL: call_protected_func:
+; CHECK: s_getpc_b64 s{{\[}}[[PC_LO:[0-9]+]]:[[PC_HI:[0-9]+]]{{\]}}
+; CHECK: s_add_u32 s[[ADDR_LO:[0-9]+]], s[[PC_LO]], protected_func@rel32@lo+4
+; CHECK: s_addc_u32 s[[ADDR_HI:[0-9]+]], s[[PC_HI]], protected_func@rel32@hi+4
+; CHECK: s_swappc_b64 s{{\[}}{{[0-9]+:[0-9]+}}{{\]}}, s{{\[}}[[ADDR_LO]]:[[ADDR_HI]]{{\]}}
+define amdgpu_kernel void @call_protected_func(i32 addrspace(1)* %out) {
+  call void @protected_func(i32 addrspace(1)* %out)
+  ret void
+}
+
+; CHECK-LABEL: call_hidden_func:
+; CHECK: s_getpc_b64 s{{\[}}[[PC_LO:[0-9]+]]:[[PC_HI:[0-9]+]]{{\]}}
+; CHECK: s_add_u32 s[[ADDR_LO:[0-9]+]], s[[PC_LO]], hidden_func@rel32@lo+4
+; CHECK: s_addc_u32 s[[ADDR_HI:[0-9]+]], s[[PC_HI]], hidden_func@rel32@hi+4
+; CHECK: s_swappc_b64 s{{\[}}{{[0-9]+:[0-9]+}}{{\]}}, s{{\[}}[[ADDR_LO]]:[[ADDR_HI]]{{\]}}
+define amdgpu_kernel void @call_hidden_func(i32 addrspace(1)* %out) {
+  call void @hidden_func(i32 addrspace(1)* %out)
+  ret void
+}
+
+attributes #0 = { noinline }