AMDGPU: Don't put constants in .text for Mesa

[cwabbott]

Summary

In Mesa we'd like to start creating constant globals. These should be in
the CONSTANT address space to let us use scalar loads where possible.
However, we also have to support pasting multiple text sections
together for RadeonSI, which means that we need to have constants in a
separate .rodata section so that they can get relocated after all the
pasted-together .text sections. We don't need this for radv, but it
shouldn't hurt either.

[cwabbott]

FYI, I think I don't have commit access anymore because of the whole licensing thing, so I'll need someone else to commit for me.

[arsenm]

Is mesa actually using mesa3d now? I thought we were still in the unpleasant situation where "unknown" OS was treated like mesa3d, but not quite. I thought clover and radv were using mesa3d, but OpenGL was not. If it has, can we finally drop the second scratch implementation?

[cwabbott]

In D67003#1652676, @arsenm wrote:

Is mesa actually using mesa3d now? I thought we were still in the unpleasant situation where "unknown" OS was treated like mesa3d, but not quite. I thought clover and radv were using mesa3d, but OpenGL was not. If it has, can we finally drop the second scratch implementation?

Ugh, you're right, radeonsi still has target triple = "amdgcn--". Why exactly is that? Would anything break if we just changed it to amdgcn-mesa-mesa3d like what radv does? I don't want that to block this, though, so is there any other user that needs to return true here? AMDVLK maybe?

[arsenm]

In D67003#1652686, @cwabbott wrote:

In D67003#1652676, @arsenm wrote:

Is mesa actually using mesa3d now? I thought we were still in the unpleasant situation where "unknown" OS was treated like mesa3d, but not quite. I thought clover and radv were using mesa3d, but OpenGL was not. If it has, can we finally drop the second scratch implementation?

Ugh, you're right, radeonsi still has target triple = "amdgcn--". Why exactly is that? Would anything break if we just changed it to amdgcn-mesa-mesa3d like what radv does? I don't want that to block this, though, so is there any other user that needs to return true here? AMDVLK maybe?

Mesa is the only oddball relying on the unknown OS behavior. IIRC this required work in mesa to switch the scratch ABI from using relocations to what radv does, but besides that I think everything would work

[cwabbott]

In D67003#1652687, @arsenm wrote:

In D67003#1652686, @cwabbott wrote:

In D67003#1652676, @arsenm wrote:

Is mesa actually using mesa3d now? I thought we were still in the unpleasant situation where "unknown" OS was treated like mesa3d, but not quite. I thought clover and radv were using mesa3d, but OpenGL was not. If it has, can we finally drop the second scratch implementation?

Ugh, you're right, radeonsi still has target triple = "amdgcn--". Why exactly is that? Would anything break if we just changed it to amdgcn-mesa-mesa3d like what radv does? I don't want that to block this, though, so is there any other user that needs to return true here? AMDVLK maybe?

Mesa is the only oddball relying on the unknown OS behavior. IIRC this required work in mesa to switch the scratch ABI from using relocations to what radv does, but besides that I think everything would work

Ok, but what I wanted to ask is whether just deleting this function and just always using .rodata is feasible, i.e. whether the only other user (AMDVLK) depends on sticking constant data in .text.

[arsenm]

In D67003#1652704, @cwabbott wrote:

In D67003#1652687, @arsenm wrote:

In D67003#1652686, @cwabbott wrote:

In D67003#1652676, @arsenm wrote:

Is mesa actually using mesa3d now? I thought we were still in the unpleasant situation where "unknown" OS was treated like mesa3d, but not quite. I thought clover and radv were using mesa3d, but OpenGL was not. If it has, can we finally drop the second scratch implementation?

Ugh, you're right, radeonsi still has target triple = "amdgcn--". Why exactly is that? Would anything break if we just changed it to amdgcn-mesa-mesa3d like what radv does? I don't want that to block this, though, so is there any other user that needs to return true here? AMDVLK maybe?

Mesa is the only oddball relying on the unknown OS behavior. IIRC this required work in mesa to switch the scratch ABI from using relocations to what radv does, but besides that I think everything would work

Ok, but what I wanted to ask is whether just deleting this function and just always using .rodata is feasible, i.e. whether the only other user (AMDVLK) depends on sticking constant data in .text.

I don't think it depends on it, but I'm not sure @dstuttard @tpr @nhaehnle

[cwabbott]

I just noticed that this already came up in D65813 and it does the right thing, it's just waiting review.

[tpr]

In D67003#1654339, @cwabbott wrote:

I just noticed that this already came up in D65813 and it does the right thing, it's just waiting review.

Hi Connor. Are you able to review D65813 for Jay please? Thanks.

[nhaehnle]

FWIW, the reason that radeonsi uses amdgcn-- is the scratch buffer ABI.

llvm/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::AMDHSA && TT.getOS() != Triple::Mesa3D;
 }
 
 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/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 -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NONE %s
+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=MESAHSA %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=MESAHSA -check-prefix=HSA %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
+; Non-HSA or Mesa OSes use fixup into .text section.
+; NONE: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1
+; NONE: s_addc_u32 s{{[0-9]+}}, s[[PC0_HI]], 0
+
+; HSA and Mesa use relocations.
+; MESAHSA: s_add_u32 s{{[0-9]+}}, s[[PC0_LO]], private1@rel32@lo+4
+; MESAHSA: 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
+; Non-HSA or Mesa OSes use fixup into .text section.
+; NONE: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2
+; NONE: s_addc_u32 s{{[0-9]+}}, s[[PC1_HI]], 0
+
+; HSA and Mesa use relocations.
+; MESAHSA: s_add_u32 s{{[0-9]+}}, s[[PC1_LO]], private2@rel32@lo+4
+; MESAHSA: 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