Partially revert D61491 "AMDGPU: Be explicit about whether the high-word in SI_PC_ADD_REL_OFFSET is 0"

[foad]

Summary

D61491 caused us to use relocs when they're not strictly necessary, to
refer to symbols in the text section. This is a pessimization and it's a
problem for some loaders that don't support relocs yet.

[nhaehnle]

Hmm, those test changes were unexpected.

Could you please change shouldEmitConstantsToTextSection to check for == AMDPAL instead? This really doesn't make sense for Mesa... and then add a test that explicit sets amdpal in the triple?

[foad]

Only put constants in the text section on PAL.

[foad]

In D65813#1618503, @nhaehnle wrote:

Could you please change shouldEmitConstantsToTextSection to check for == AMDPAL instead? This really doesn't make sense for Mesa... and then add a test that explicit sets amdpal in the triple?

Done. global-constant.ll seems to test the HAS/non-HSA (now PAL/non-PAL) difference quite well. I don't know what to make of the other test changes -- I just did what I needed to to make them pass.

[foad]

Ping.

[foad]

Ping!

[cwabbott]

FWIW, this LGTM. Maybe it would be a good idea to add more lines to global-constant.ll to test radeonsi (amdgcn-- at the moment) and radv (amdgcn-mesa-mesa3d) under the NOPAL label.

I believe the no-initializer-constant-addrspace.ll test change is because it uses the external linkage and a non-PAL OS, so we emit an extra global offset table lookup once the workaround is removed, but that's normal and shouldn't matter for what it's testing. The R600 change is just due to it getting a separate section and also seems fine.

llvm/trunk/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.
-  unsigned LoFlags = GAFlags;
-  if (LoFlags == SIInstrInfo::MO_NONE)
-    LoFlags = SIInstrInfo::MO_REL32;
   SDValue PtrLo =
-      DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4, LoFlags);
+      DAG.getTargetGlobalAddress(GV, DL, MVT::i32, Offset + 4, GAFlags);
   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);

llvm/trunk/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

 }
 
 bool isReadOnlySegment(const GlobalValue *GV) {
   return GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS ||
          GV->getType()->getAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS_32BIT;
 }
 
 bool shouldEmitConstantsToTextSection(const Triple &TT) {
-  return TT.getOS() != Triple::AMDHSA;
+  return TT.getOS() == Triple::AMDPAL;
 }
 
 int getIntegerAttribute(const Function &F, StringRef Name, int Default) {
   Attribute A = F.getFnAttribute(Name);
   int Result = Default;
 
   if (A.isStringAttribute()) {
     StringRef Str = A.getValueAsString();

llvm/trunk/test/CodeGen/AMDGPU/global-constant.ll

-; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NOHSA %s
-; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=HSA %s
+; RUN: llc -mtriple=amdgcn--amdpal -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=PAL %s
+; RUN: llc -mtriple=amdgcn-- -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NOPAL %s
+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NOPAL %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NOPAL %s
 
 @private1 = private unnamed_addr addrspace(4) constant [4 x float] [float 0.0, float 1.0, float 2.0, float 3.0]
 @private2 = private unnamed_addr addrspace(4) constant [4 x float] [float 4.0, float 5.0, float 6.0, float 7.0]
 @available_externally = available_externally addrspace(4) global [256 x i32] zeroinitializer
 
 ; GCN-LABEL: {{^}}private_test:
 ; GCN: s_getpc_b64 s{{\[}}[[PC0_LO:[0-9]+]]:[[PC0_HI:[0-9]+]]{{\]}}
 
-; Non-HSA OSes use fixup into .text section.
-; NOHSA: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1
-; NOHSA: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], 0
-
-; HSA OSes use relocations.
-; HSA: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1@rel32@lo+4
-; HSA: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], private1@rel32@hi+4
+; PAL OSes use fixup into .text section.
+; PAL: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1
+; PAL: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], 0
+
+; Non-PAL OSes use relocations.
+; NOPAL: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1@rel32@lo+4
+; NOPAL: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], private1@rel32@hi+4
 
 ; GCN: s_getpc_b64 s{{\[}}[[PC1_LO:[0-9]+]]:[[PC1_HI:[0-9]+]]{{\]}}
 
-; Non-HSA OSes use fixup into .text section.
-; NOHSA: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2
-; NOHSA: s_addc_u32 s{{[0-9]+}}, s[[PC1_HI]], 0
-
-; HSA OSes use relocations.
-; HSA: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2@rel32@lo+4
-; HSA: s_addc_u32 s{{[0-9]+}}, s[[PC1_HI]], private2@rel32@hi+4
+; PAL OSes use fixup into .text section.
+; PAL: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2
+; PAL: s_addc_u32 s{{[0-9]+}}, s[[PC1_HI]], 0
+
+; Non-PAL OSes use relocations.
+; NOPAL: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2@rel32@lo+4
+; NOPAL: s_addc_u32 s{{[0-9]+}}, s[[PC1_HI]], private2@rel32@hi+4
 
 define amdgpu_kernel void @private_test(i32 %index, float addrspace(1)* %out) {
   %ptr = getelementptr [4 x float], [4 x float] addrspace(4) * @private1, i32 0, i32 %index
   %val = load float, float addrspace(4)* %ptr
   store volatile float %val, float addrspace(1)* %out
   %ptr2 = getelementptr [4 x float], [4 x float] addrspace(4) * @private2, i32 0, i32 %index
   %val2 = load float, float addrspace(4)* %ptr2
   store volatile float %val2, float addrspace(1)* %out
   ret void
 }
 
-; HSA-LABEL: {{^}}available_externally_test:
-; HSA: s_getpc_b64 s{{\[}}[[PC0_LO:[0-9]+]]:[[PC0_HI:[0-9]+]]{{\]}}
-; HSA: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], available_externally@gotpcrel32@lo+4
-; HSA: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], available_externally@gotpcrel32@hi+4
+; NOPAL-LABEL: {{^}}available_externally_test:
+; NOPAL: s_getpc_b64 s{{\[}}[[PC0_LO:[0-9]+]]:[[PC0_HI:[0-9]+]]{{\]}}
+; NOPAL: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], available_externally@gotpcrel32@lo+4
+; NOPAL: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], available_externally@gotpcrel32@hi+4
 define amdgpu_kernel void @available_externally_test(i32 addrspace(1)* %out) {
   %ptr = getelementptr [256 x i32], [256 x i32] addrspace(4)* @available_externally, i32 0, i32 1
   %val = load i32, i32 addrspace(4)* %ptr
   store i32 %val, i32 addrspace(1)* %out
   ret void
 }
 
-; NOHSA: .text
-; HSA: .section .rodata
+; PAL: .text
+; NOPAL: .section .rodata
 
 ; GCN: private1:
 ; GCN: private2:

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

 ; RUN: llc -march=amdgcn -mcpu=tahiti -filetype=obj < %s | llvm-readobj -r --symbols | FileCheck %s -check-prefix=GCN
 ; RUN: llc -march=amdgcn -mcpu=tonga -filetype=obj < %s | llvm-readobj -r --symbols | FileCheck %s -check-prefix=GCN
 ; RUN: llc -march=r600 -mcpu=cypress -filetype=obj < %s | llvm-readobj -r --symbols | FileCheck %s -check-prefix=EG
 
-; GCN: R_AMDGPU_REL32_LO extern_const_addrspace
+; GCN: R_AMDGPU_GOTPCREL32_LO extern_const_addrspace 0x0
 ; 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

llvm/trunk/test/CodeGen/AMDGPU/r600-constant-array-fixup.ll

 ; RUN: llc -filetype=obj -march=r600 -mcpu=cypress -verify-machineinstrs < %s | llvm-readobj -r --symbols | FileCheck %s
 
 @arr = internal unnamed_addr addrspace(4) constant [4 x i32] [i32 4, i32 5, i32 6, i32 7], align 4
 
 ; CHECK: Relocations [
 ; CHECK: Section (3) .rel.text {
-; CHECK: 0x58 R_AMDGPU_ABS32 .text 0x0
+; CHECK: 0x58 R_AMDGPU_ABS32 arr 0x0
 ; CHECK: }
 ; CHECK: ]
 
 ; CHECK: Symbol {
 ; CHECK: Name: arr (11)
-; CHECK: Value: 0x70
+; CHECK: Value: 0x0
 ; CHECK: Size: 16
 ; CHECK: Binding: Local (0x0)
 ; CHECK: Type: Object (0x1)
 ; CHECK: Other: 0
 ; CHECK: Section: .text (0x2)
 ; CHECK: }
 define amdgpu_kernel void @test_constant_array_fixup(i32 addrspace(1)* nocapture %out, i32 %idx) #0 {
 entry:
   %arrayidx = getelementptr inbounds [4 x i32], [4 x i32] addrspace(4)* @arr, i32 0, i32 %idx
   %val = load i32, i32 addrspace(4)* %arrayidx
   store i32 %val, i32 addrspace(1)* %out, align 4
   ret void
 }
 
 attributes #0 = { nounwind }