AMDGPU: Be explicit about whether the high-word in SI_PC_ADD_REL_OFFSET is 0

[nhaehnle]

Summary

Instead of encoding a high-word of 0 using a fake TargetGlobalAddress,
just use a literal target constant. This simplifies some subsequent changes.

The generated assembly is now more explicit about the kind of relocation
that is to be used.

Change-Id: I066835202d23b5941fa7a358eb4b89e9b71ab6f8

[arsenm]

But these are often -1?

[nhaehnle]

In D61491#1489425, @arsenm wrote:

But these are often -1?

Not sure what you're saying here? The point is that when the GAFlags are MO_NONE, there was previously magic behavior to force the high dword to 0. This patch removes the magic behavior of MO_NONE, or perhaps it's more accurate to say that it pushes the magic behavior forward into buildPCRelGlobalAddress, so that it's confined locally. The MO_NONE case is only hit in the badly named shouldEmitFixup case, which applies to constants that should be emitted to the .text section, where they will always be after the code.

(The shouldEmitConstantsToTextSection is itself a gross hack that is applied based on the triple. We'll soon be able to remove it for Mesa, once the proper linking lands, and then hopefully later on for PAL as well.)

[nhaehnle]

ping?

[nhaehnle]

ping

[arsenm]

LGTM. Are you saying we should remove shouldEmitFixup and just always use buildPCRelGlobalAddress?

[nhaehnle]

In D61491#1533471, @arsenm wrote:

Are you saying we should remove shouldEmitFixup and just always use buildPCRelGlobalAddress?

The current trunk always uses buildPCRelGlobalAddress, and that makes sense to me. shouldEmitFixup is part of the logic that determines what kind of PC-relative global address is used. There seem to be three modes right now:

1. 64-bit PC-relative to the symbol
2. 32-bit PC-relative to the symbol
3. 64-bit PC-relative to the GOT, loading the final symbol address from the GOT.

The logic deciding between these cases seems like it could benefit from being streamlined and simplified, but I wouldn't remove it outright as the distinction can be helpful.

lib/Target/AMDGPU/SIISelLowering.cpp

buildPCRelGlobalAddress(SelectionDAG &DAG, const GlobalValue *GV,
   //   operand to the global variable.
   //
   // What we want here is an offset from the value returned by s_getpc
   // (which is the address of the s_add_u32 instruction) to the global
   // variable, but since the encoding of $symbol starts 4 bytes after the start
   // of the s_add_u32 instruction, we end up with an offset that is 4 bytes too
   // small. This requires us to add 4 to the global variable offset in order to
   // compute the correct address.
-  SDValue PtrLo = DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4,
-                                             GAFlags);
-  SDValue PtrHi = DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4,
-                                             GAFlags == SIInstrInfo::MO_NONE ?
-                                             GAFlags : GAFlags + 1);
+  unsigned LoFlags = GAFlags;
+  if (LoFlags == SIInstrInfo::MO_NONE)
+    LoFlags = SIInstrInfo::MO_REL32;
+  SDValue PtrLo =
+      DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4, LoFlags);
+  SDValue PtrHi;
+  if (GAFlags == SIInstrInfo::MO_NONE) {
+    PtrHi = DAG.getTargetConstant(0, DL, MVT::i32);
+  } else {
+    PtrHi =
+        DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4, GAFlags + 1);
+  }
   return DAG.getNode(AMDGPUISD::PC_ADD_REL_OFFSET, DL, PtrVT, PtrLo, PtrHi);
 }
 
 SDValue SITargetLowering::LowerGlobalAddress(AMDGPUMachineFunction *MFI,
                                              SDValue Op,
                                              SelectionDAG &DAG) const {
   GlobalAddressSDNode *GSD = cast<GlobalAddressSDNode>(Op);
   const GlobalValue *GV = GSD->getGlobal();

lib/Target/AMDGPU/SIInstrInfo.cpp

case AMDGPU::SI_PC_ADD_REL_OFFSET: {
     // Add 32-bit offset from this instruction to the start of the
     // constant data.
     Bundler.append(BuildMI(MF, DL, get(AMDGPU::S_ADD_U32), RegLo)
                        .addReg(RegLo)
                        .add(MI.getOperand(1)));
 
     MachineInstrBuilder MIB = BuildMI(MF, DL, get(AMDGPU::S_ADDC_U32), RegHi)
                                   .addReg(RegHi);
-    if (MI.getOperand(2).getTargetFlags() == SIInstrInfo::MO_NONE)
-      MIB.addImm(0);
-    else
-      MIB.add(MI.getOperand(2));
 
     Bundler.append(MIB);
     finalizeBundle(MBB, Bundler.begin());
 
     MI.eraseFromParent();
     break;
   }
   case AMDGPU::ENTER_WWM: {

lib/Target/AMDGPU/SIInstructions.td

 defm SI_SPILL_V160 : SI_SPILL_VGPR <VReg_160>;
 defm SI_SPILL_V256 : SI_SPILL_VGPR <VReg_256>;
 defm SI_SPILL_V512 : SI_SPILL_VGPR <VReg_512>;
 
 def SI_PC_ADD_REL_OFFSET : SPseudoInstSI <
   (outs SReg_64:$dst),
   (ins si_ga:$ptr_lo, si_ga:$ptr_hi),
   [(set SReg_64:$dst,
-   (i64 (SIpc_add_rel_offset (tglobaladdr:$ptr_lo), (tglobaladdr:$ptr_hi))))]> {
+      (i64 (SIpc_add_rel_offset tglobaladdr:$ptr_lo, tglobaladdr:$ptr_hi)))]> {
   let Defs = [SCC];
 }
 
 def : GCNPat <
+  (SIpc_add_rel_offset tglobaladdr:$ptr_lo, 0),
+  (SI_PC_ADD_REL_OFFSET $ptr_lo, (i32 0))
+>;
+
+def : GCNPat <
   (AMDGPUinit_exec i64:$src),
   (SI_INIT_EXEC (as_i64imm $src))
 >;
 
 def : GCNPat <
   (AMDGPUinit_exec_from_input i32:$input, i32:$shift),
   (SI_INIT_EXEC_FROM_INPUT (i32 $input), (as_i32imm $shift))
 >;

test/CodeGen/AMDGPU/llvm.memcpy.ll

 }
 
 ; Test shouldConvertConstantLoadToIntImm
 @hello.align4 = private unnamed_addr addrspace(4) constant [16 x i8] c"constant string\00", align 4
 @hello.align1 = private unnamed_addr addrspace(4) constant [16 x i8] c"constant string\00", align 1
 
 ; FUNC-LABEL: {{^}}test_memcpy_const_string_align4:
 ; SI: s_getpc_b64
-; SI: s_add_u32 s{{[0-9]+}}, s{{[0-9]+}}, hello.align4+20
+; SI: s_add_u32 s{{[0-9]+}}, s{{[0-9]+}}, hello.align4@rel32@lo+20
 ; SI: s_addc_u32
 ; SI-DAG: s_load_dwordx4
 ; SI-DAG: s_load_dwordx4
 ; SI-DAG: s_load_dwordx2
 ; SI-DAG: buffer_store_dwordx4
 ; SI-DAG: buffer_store_dwordx4
 define amdgpu_kernel void @test_memcpy_const_string_align4(i8 addrspace(1)* noalias %out) nounwind {
   %str = bitcast [16 x i8] addrspace(4)* @hello.align4 to i8 addrspace(4)*

test/CodeGen/AMDGPU/no-initializer-constant-addrspace.ll

 ; RUN: llc -march=amdgcn -mcpu=tahiti -filetype=obj < %s | llvm-readobj -relocations -symbols | FileCheck %s -check-prefix=GCN
 ; RUN: llc -march=amdgcn -mcpu=tonga -filetype=obj < %s | llvm-readobj -relocations -symbols | FileCheck %s -check-prefix=GCN
 ; RUN: llc -march=r600 -mcpu=cypress -filetype=obj < %s | llvm-readobj -relocations -symbols | FileCheck %s -check-prefix=EG
 
-; GCN: R_AMDGPU_REL32 extern_const_addrspace
+; GCN: R_AMDGPU_REL32_LO extern_const_addrspace
 ; EG: R_AMDGPU_ABS32 extern_const_addrspace
 
 ; CHECK-DAG: Name: extern_const_addrspace
 @extern_const_addrspace = external unnamed_addr addrspace(4) constant [5 x i32], align 4
 
 ; CHECK-DAG: Name: load_extern_const_init
 define amdgpu_kernel void @load_extern_const_init(i32 addrspace(1)* %out) nounwind {
   %val = load i32, i32 addrspace(4)* getelementptr ([5 x i32], [5 x i32] addrspace(4)* @extern_const_addrspace, i64 0, i64 3), align 4