[HIP] Add -fvisibility hidden option to clang

[yaxunl]

Summary

AMDGPU target need -fvisibility hidden option for clang to
work around a limitation of no PLT support, otherwise there is compilation
error at -O0.

[tra]

Could you elaborate on what exactly is the problem this patch fixes?
I don't see how internalizing the symbols connects to PLTs. My understanding is that PLTs are used to provide stubs for symbols to be resolved by dynamic linker at runtime. AFAICT AMD does not use shared libs on device side. What do I miss?

[yaxunl]

In D51434#1217772, @tra wrote:

Could you elaborate on what exactly is the problem this patch fixes?
I don't see how internalizing the symbols connects to PLTs. My understanding is that PLTs are used to provide stubs for symbols to be resolved by dynamic linker at runtime. AFAICT AMD does not use shared libs on device side. What do I miss?

When AMDGPU backend generates ELF containing device ISA's, any non-kernel functions require
PLT support to be emitted to ELF. However AMDGPU backend currently does not support that,
which causes error.

Internalization will eliminate any non-kernel functions, therefore works around this limitation.

[tra]

I could not find anything about PLTs in AMDGPU-ABI, nor could I find anything relevant on google.
I still have no idea why PLTs are required in this case. Without that info, the problem may as well be due to unintended requirement for PLT that this patch would hide.

I'm going to defer to someone more familiar with amdgpu to tell whether that's the right fix for the problem.

[scott.linder]

+Matt to confirm, but our executable format is a shared object and we eventually want to support preemptible symbols through the PLT. We already generate GOT entries for globals.

Currently we work around the lack of PLT support by internalizing all non-kernel functions, so for now this patch is required.

[arsenm]

I have a patch to change the default visibility which I think is a better option

[arsenm]

D51209 is the patch. I think HIP will need an additional patch, since I think it isn’t subclassing the amdgpu toolchain

[yaxunl]

In D51434#1217971, @arsenm wrote:

D51209 is the patch. I think HIP will need an additional patch, since I think it isn’t subclassing the amdgpu toolchain

Yes since HIP has different toolchain. This does not affect kernel's visibility, right?

[arsenm]

In D51434#1218017, @yaxunl wrote:

In D51434#1217971, @arsenm wrote:

D51209 is the patch. I think HIP will need an additional patch, since I think it isn’t subclassing the amdgpu toolchain

Yes since HIP has different toolchain. This does not affect kernel's visibility, right?

It does, but the visibility of the kernel shouldn't matter

[yaxunl]

Use -fvisibility hidden.

[tra]

lib/Driver/ToolChains/HIP.cpp

255 ↗

(On Diff #163186)

Nit: You could collapse multiple push_back calls into a single append({...}):
CC1Args.append({"-fvisibility", "hidden"});

[yaxunl]

Revised by Artem's comments.

[arsenm]

lib/Driver/ToolChains/HIP.cpp

256 ↗

(On Diff #163196)

We should probably start subclassing the HIP toolchain from AMDGPU and share more of this

[yaxunl]

lib/Driver/ToolChains/HIP.cpp

256 ↗

(On Diff #163196)

I'd like to defer the refactoring of HIP and AMDGPU toolchain to another patch. There are stuff in HIP toolchain which should go to AMDGPU toolchain. We need to sort out the relation between HIP and AMDGPU toolchain in that patch.

[arsenm]

lib/Driver/ToolChains/HIP.cpp

256 ↗

(On Diff #163196)

Yes

[arsenm]

lib/Driver/ToolChains/HIP.cpp

256 ↗

(On Diff #163196)

I also have a patch refactoring the AMDGPU toolchain that would conflict

[yaxunl]

lib/Driver/ToolChains/HIP.cpp

256 ↗

(On Diff #163196)

I will commit this patch then we will start reviewing your patch for refactoring AMDGPU toolchain.

cfe/trunk/lib/Driver/ToolChains/HIP.cpp

void HIPToolChain::addClangTargetOptions(
 
   if (DriverArgs.hasFlag(options::OPT_fcuda_approx_transcendentals,
                          options::OPT_fno_cuda_approx_transcendentals, false))
     CC1Args.push_back("-fcuda-approx-transcendentals");
 
   if (DriverArgs.hasFlag(options::OPT_fcuda_rdc, options::OPT_fno_cuda_rdc,
                          false))
     CC1Args.push_back("-fcuda-rdc");
+
+  // Default to "hidden" visibility, as object level linking will not be
+  // supported for the foreseeable future.
+  if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,
+                         options::OPT_fvisibility_ms_compat))
+    CC1Args.append({"-fvisibility", "hidden"});
 }
 
 llvm::opt::DerivedArgList *
 HIPToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
                              StringRef BoundArch,
                              Action::OffloadKind DeviceOffloadKind) const {
   DerivedArgList *DAL =
       HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind);

cfe/trunk/test/Driver/hip-toolchain.hip

 // RUN:   -fuse-ld=lld \
 // RUN:   %S/Inputs/hip_multiple_inputs/a.cu \
 // RUN:   %S/Inputs/hip_multiple_inputs/b.hip \
 // RUN: 2>&1 | FileCheck %s
 
 // CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa" 
 // CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
 // CHECK-SAME: {{.*}} "-main-file-name" "a.cu" {{.*}} "-target-cpu" "gfx803"
-// CHECK-SAME: "-fcuda-is-device" {{.*}} "-o" [[A_BC:".*bc"]] "-x" "hip"
+// CHECK-SAME: "-fcuda-is-device" "-fvisibility" "hidden"
+// CHECK-SAME: {{.*}} "-o" [[A_BC:".*bc"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[A_SRC:".*a.cu"]]
 
 // CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
 // CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
 // CHECK-SAME: {{.*}} "-main-file-name" "b.hip" {{.*}} "-target-cpu" "gfx803"
-// CHECK-SAME: "-fcuda-is-device" {{.*}} "-o" [[B_BC:".*bc"]] "-x" "hip"
+// CHECK-SAME: "-fcuda-is-device" "-fvisibility" "hidden"
+// CHECK-SAME: {{.*}} "-o" [[B_BC:".*bc"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[B_SRC:".*b.hip"]]
 
 // CHECK: [[LLVM_LINK:"*.llvm-link"]] [[A_BC]] [[B_BC]]
 // CHECK-SAME: "{{.*}}lib1.bc" "{{.*}}lib2.bc"
 // CHECK-SAME: "-o" [[LINKED_BC_DEV1:".*-gfx803-linked-.*bc"]]
 
 // CHECK: [[OPT:".*opt"]] [[LINKED_BC_DEV1]] "-mtriple=amdgcn-amd-amdhsa"
 // CHECK-SAME: "-mcpu=gfx803"