[PATCH] D57416: [AMDGPU] Support emitting GOT relocations for function calls

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked an inline comment as done.
scott.linder added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:3488-3492
+    if (DefI->getOpcode() == AMDGPU::S_LOAD_DWORDX2_IMM) {
+      AddrReg = DefI->getOperand(1).getReg();
+      DefI = MRI.getVRegDef(AddrReg);
+    }
+    assert(DefI->getOpcode() == AMDGPU::SI_PC_ADD_REL_OFFSET);
----------------
arsenm wrote:
> scott.linder wrote:
> > arsenm wrote:
> > > Looking for a specific load opcode looks concerning. Where is this coming from? I would think we need a pseudo at this point
> > 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.
> 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.
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?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57416/new/

https://reviews.llvm.org/D57416