[HIP] Allow undefined symbols

[yaxunl]

Summary

HIP managed variables need to be emitted as undefined symbols
since runtime needs to define them with managed memory
accessible by both device and host.

Let HIP toolchain allow that with lld.

[tra]

What's going to happen if you do have an undefined reference that's *not* to a __managed__ variable?

[yaxunl]

In D95970#2540303, @tra wrote:

What's going to happen if you do have an undefined reference that's *not* to a __managed__ variable?

By default HIP toolchain uses -fvisibility hidden -fapply-global-visibility-to-externs for device compilation. Undefined variable symbols have protected visibility. Undefined function symbols have hidden visibility. Since they are not allowed to be preempted, lld still emits error if they are undefined. __managed__ variables have default visibility, therefore they are allowed to go through.

We can let HIP runtime emit an error if there are undefined symbols other than managed variables.

[tra]

In D95970#2540414, @yaxunl wrote:

In D95970#2540303, @tra wrote:

What's going to happen if you do have an undefined reference that's *not* to a __managed__ variable?

By default HIP toolchain uses -fvisibility hidden -fapply-global-visibility-to-externs for device compilation. Undefined variable symbols have protected visibility. Undefined function symbols have hidden visibility. Since they are not allowed to be preempted, lld still emits error if they are undefined. __managed__ variables have default visibility, therefore they are allowed to go through.

Is there a test that verifies/demonstrates how it's handled at compile time?

We can let HIP runtime emit an error if there are undefined symbols other than managed variables.

Detecting compilation errors at run-time is not very useful. The end user would likely have no idea what's going on.
I wonder if we can catch the errors earlier. E.g. w/o RDC, we can probably catch it in the codegen.
With RDC, only the linker will know it, so you would need some sort of custom plugin for that.

What if we actually resolve all __managed__ references and point them to some sort of placeholder.
E.g a weak symbol, that would be directed to the real location when the binary is loaded.
Or, maybe, introduce an indirection and let the runtime handle managed variables as a special case
and point the indirect links to the right locations.
This way you can still run with -no-undefined and find all other unresolved references that should not be there.

[yaxunl]

In D95970#2540669, @tra wrote:

In D95970#2540414, @yaxunl wrote:

In D95970#2540303, @tra wrote:

What's going to happen if you do have an undefined reference that's *not* to a __managed__ variable?

By default HIP toolchain uses -fvisibility hidden -fapply-global-visibility-to-externs for device compilation. Undefined variable symbols have protected visibility. Undefined function symbols have hidden visibility. Since they are not allowed to be preempted, lld still emits error if they are undefined. __managed__ variables have default visibility, therefore they are allowed to go through.

Is there a test that verifies/demonstrates how it's handled at compile time?

We can let HIP runtime emit an error if there are undefined symbols other than managed variables.

Detecting compilation errors at run-time is not very useful. The end user would likely have no idea what's going on.
I wonder if we can catch the errors earlier. E.g. w/o RDC, we can probably catch it in the codegen.
With RDC, only the linker will know it, so you would need some sort of custom plugin for that.

What if we actually resolve all __managed__ references and point them to some sort of placeholder.
E.g a weak symbol, that would be directed to the real location when the binary is loaded.
Or, maybe, introduce an indirection and let the runtime handle managed variables as a special case
and point the indirect links to the right locations.
This way you can still run with -no-undefined and find all other unresolved references that should not be there.

Good point. I will try using indirection. This should avoid undefined symbols in device binary.

clang/lib/Driver/ToolChains/HIP.cpp

 } // namespace
 
 void AMDGCN::Linker::constructLldCommand(Compilation &C, const JobAction &JA,
                                           const InputInfoList &Inputs,
                                           const InputInfo &Output,
                                           const llvm::opt::ArgList &Args) const {
   // Construct lld command.
   // The output from ld.lld is an HSA code object file.
-  ArgStringList LldArgs{"-flavor", "gnu", "--no-undefined", "-shared",
+  // Do not use --no-undefined option since managed variables need to be emitted
+  // as undefined symbols.
+  ArgStringList LldArgs{"-flavor", "gnu", "-shared",
                         "-plugin-opt=-amdgpu-internalize-symbols"};
 
   auto &TC = getToolChain();
   auto &D = TC.getDriver();
   assert(!Inputs.empty() && "Must have at least one input.");
   addLTOOptions(TC, Args, LldArgs, Output, Inputs[0],
                 D.getLTOMode() == LTOK_Thin);
 

clang/test/Driver/hip-toolchain-device-only.hip

 // REQUIRES: clang-driver, amdgpu-registered-target
 
 // RUN: %clang -### -target x86_64-linux-gnu \
 // RUN:   --offload-arch=gfx803 --offload-arch=gfx900 \
 // RUN:   --cuda-device-only -nogpuinc -nogpulib -c \
 // RUN:   %s 2>&1 | FileCheck %s
 
 // CHECK-NOT: error:
 
 // CHECK: [[CLANG:".*clang.*"]] "-cc1"{{.*}} "-triple" "amdgcn-amd-amdhsa"
 // CHECK-SAME: "-fcuda-is-device"
 // CHECK-SAME: "-target-cpu" "gfx803"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_A_803:".*o"]] "-x" "hip"
 
-// CHECK: [[LLD: ".*lld.*"]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD: ".*lld.*"]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_803:.*out]]" [[OBJ_DEV_A_803]]
 
 // CHECK: [[CLANG:".*clang.*"]] "-cc1"{{.*}} "-triple" "amdgcn-amd-amdhsa"
 // CHECK-SAME: "-emit-obj"
 // CHECK-SAME: "-fcuda-is-device"
 // CHECK-SAME: "-target-cpu" "gfx900"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_A_900:".*o"]] "-x" "hip"
 
-// CHECK: [[LLD]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_900:.*out]]" [[OBJ_DEV_A_900]]
 
 // CHECK: [[BUNDLER:".*clang-offload-bundler"]] "-type=o"
 // CHECK-SAME: "-targets={{.*}},hip-amdgcn-amd-amdhsa--gfx803,hip-amdgcn-amd-amdhsa--gfx900"
 // CHECK-SAME: "-inputs={{.*}},[[IMG_DEV_A_803]],[[IMG_DEV_A_900]]" "-outputs=[[BUNDLE_A:.*hipfb]]"

clang/test/Driver/hip-toolchain-no-rdc.hip

 // CHECK-SAME: "-target-cpu" "gfx803"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_A_803:".*o"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[A_SRC:".*a.cu"]]
 
 // CHECK-NOT: {{".*llvm-link"}}
 // CHECK-NOT: {{".*opt"}}
 // CHECK-NOT: {{".*llc"}}
 
-// CHECK: [[LLD: ".*lld.*"]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD: ".*lld.*"]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_803:.*out]]" [[OBJ_DEV_A_803]]
 
 //
 // Compile device code in a.cu to code object for gfx900.
 //
 
 // CHECK: [[CLANG:".*clang.*"]] "-cc1" "-mllvm" "--amdhsa-code-object-version={{[0-9]+}}" "-triple" "amdgcn-amd-amdhsa"
 // CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
 // CHECK-SAME: "-emit-obj"
 // CHECK-SAME: {{.*}} "-main-file-name" "a.cu"
 // CHECK-SAME: "-fcuda-is-device" "-mllvm" "-amdgpu-internalize-symbols"
 // CHECK-SAME: "-fcuda-allow-variadic-functions" "-fvisibility" "hidden"
 // CHECK-SAME: "-fapply-global-visibility-to-externs"
 // CHECK-SAME: "{{.*}}lib1.bc" "{{.*}}lib2.bc"
 // CHECK-SAME: "-target-cpu" "gfx900"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_A_900:".*o"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[A_SRC]]
 
 // CHECK-NOT: {{".*llvm-link"}}
 // CHECK-NOT: {{".*opt"}}
 // CHECK-NOT: {{".*llc"}}
 
-// CHECK: [[LLD]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_900:.*out]]" [[OBJ_DEV_A_900]]
 
 //
 // Bundle and embed device code in host object for a.cu.
 //
 
 // CHECK: [[BUNDLER:".*clang-offload-bundler"]] "-type=o"
 // CHECK-SAME: "-bundle-align=4096"

 // CHECK-SAME: "-target-cpu" "gfx803"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_B_803:".*o"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[B_SRC:".*b.hip"]]
 
 // CHECK-NOT: {{".*llvm-link"}}
 // CHECK-NOT: {{".*opt"}}
 // CHECK-NOT: {{".*llc"}}
 
-// CHECK: [[LLD]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_B_803:.*out]]" [[OBJ_DEV_B_803]]
 
 //
 // Compile device code in b.hip to code object for gfx900.
 //
 
 // CHECK: [[CLANG:".*clang.*"]] "-cc1" "-mllvm" "--amdhsa-code-object-version={{[0-9]+}}" "-triple" "amdgcn-amd-amdhsa"
 // CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
 // CHECK-SAME: "-emit-obj"
 // CHECK-SAME: {{.*}} "-main-file-name" "b.hip"
 // CHECK-SAME: "-fcuda-is-device" "-mllvm" "-amdgpu-internalize-symbols"
 // CHECK-SAME: "-fcuda-allow-variadic-functions" "-fvisibility" "hidden"
 // CHECK-SAME: "-fapply-global-visibility-to-externs"
 // CHECK-SAME: "{{.*}}lib1.bc" "{{.*}}lib2.bc"
 // CHECK-SAME: "-target-cpu" "gfx900"
 // CHECK-SAME: {{.*}} "-o" [[OBJ_DEV_B_900:".*o"]] "-x" "hip"
 // CHECK-SAME: {{.*}} [[B_SRC]]
 
 // CHECK-NOT: {{".*llvm-link"}}
 // CHECK-NOT: {{".*opt"}}
 // CHECK-NOT: {{".*llc"}}
 
-// CHECK: [[LLD]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_B_900:.*out]]" [[OBJ_DEV_B_900]]
 
 //
 // Bundle and embed device code in host object for b.hip.
 //
 
 // CHECK: [[BUNDLER:".*clang-offload-bundler"]] "-type=o"
 // CHECK-SAME: "-bundle-align=4096"