[AMDGPU] Add -fvisibility-amdgpu-non-kernel-functions

[scott.linder]

Summary

Controls the visibility of non-kernel functions when compiling for AMDGPU targets. Defaults to the default value visibility (-fvisibility), and is set in the AMDGPU toolchain to be "hidden" if not set explicitly.

This is a more fine-grained knob than "-fvisibility hidden", and allows the default visibility of functions to be controlled independent of kernels. This is useful for languages like OpenCL where kernel symbols are externally visible, but function symbols are not.

[scott.linder]

I don't know who else to add as a reviewer; Sam, is there someone else outside of AMD that would be interested in reviewing this?

[scott.linder]

Update docs

[arsenm]

I think the name needs work, but I'm not sure what it should be. I think it should avoid using "non" and "amdgpu"

[arsenm]

Tests should also include some global variables

[yaxunl]

In D52891#1256207, @arsenm wrote:

I think the name needs work, but I'm not sure what it should be. I think it should avoid using "non" and "amdgpu"

I think dropping amdgpu is fine since we can add (AMDGUP only) to the description of the option, following the precedence of

-ffixed-r9              Reserve the r9 register (ARM only)

However it is difficult to coin a different term for 'non-kernel-function'. Also, I saw precedence of using 'non' in option name:

-objcmt-ns-nonatomic-iosonly

So, probably we could use -fvisibility-nonkernel-function ?

[yaxunl]

Can you also fix HIP toolchain? It is in HIPToolChain::addClangTargetOptions. Thanks.

[arsenm]

Use of the word kernel might confuse general people. Maybe it needs to specify OpenCL, but it also applies to HIP/CUDA

[t-tye]

Another word commonly used across languages is "offload".

[scott.linder]

I will update the patch to modify the HIP toolchain and to add tests for global variables.

As far as the semantics are concerned, are we OK with this being AMDGPU only? I do not see a means of determining what is a "kernel" in a language-agnostic way other than checking for our AMDGPU-specific calling convention. If there is a more general mechanism, this could be implemented in LinkageComputer::getLVForNamespaceScopeDecl instead. As it stands, it sounds like being AMDGPU specific, but omitting amdgpu from the option name is preferred?

What about:

-fvisibility-non-offload-functions=<arg>

Set the default symbol visibility for non-offload function declarations (AMDGPU only)

I cannot think of a way to avoid non or something similar ending up in the name.

[yaxunl]

In D52891#1258070, @scott.linder wrote:

I will update the patch to modify the HIP toolchain and to add tests for global variables.

As far as the semantics are concerned, are we OK with this being AMDGPU only? I do not see a means of determining what is a "kernel" in a language-agnostic way other than checking for our AMDGPU-specific calling convention. If there is a more general mechanism, this could be implemented in LinkageComputer::getLVForNamespaceScopeDecl instead. As it stands, it sounds like being AMDGPU specific, but omitting amdgpu from the option name is preferred?

The checking of kernel functions can be made target independent. For now we only need to consider OpenCL and CUDA/HIP. We can check function attribute AT_CUDAGlobal and AT_OpenCLKernel. Then this option can be made target independent. HCC can add its own check out of tree.

What about:

-fvisibility-non-offload-functions=<arg>

This name looks good to me.

[arsenm]

Offload to me sounds like it decided to extract out a section of the program for offload, which is not how OpenCL works

[scott.linder]

Will be superseded by either https://reviews.llvm.org/D53153 or https://reviews.llvm.org/D56871

docs/ClangCommandLineReference.rst

 .. option:: -fvisibility-ms-compat
 
 Give global types 'default' visibility and global functions and variables 'hidden' visibility by default
 
 .. option:: -fvisibility=<arg>
 
 Set the default symbol visibility for all global declarations
 
+.. option:: -fvisibility-amdgpu-non-kernel-functions=<arg>
+
+Set the default symbol visibility for non-kernel function declarations for AMDGPU targets
+
 .. option:: -fwhole-program-vtables, -fno-whole-program-vtables
 
 Enables whole-program vtable optimization. Requires -flto
 
 .. option:: -fwrapv, -fno-wrapv
 
 Treat signed integer overflow as two's complement
 

include/clang/Basic/LangOptions.def

LANGOPT(
     "Require member pointer base types to be complete at the point where the "
     "type's inheritance model would be determined under the Microsoft ABI")
 
 ENUM_LANGOPT(GC, GCMode, 2, NonGC, "Objective-C Garbage Collection mode")
 ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,
              "value symbol visibility")
 ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
              "type symbol visibility")
+ENUM_LANGOPT(AMDGPUNonKernelFunctionVisibilityMode, Visibility, 3,
+             DefaultVisibility, "non-kernel function visibility")
 ENUM_LANGOPT(StackProtector, StackProtectorMode, 2, SSPOff,
              "stack protector mode")
 ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2, SOB_Undefined,
              "signed integer overflow handling")
 
 BENIGN_LANGOPT(ArrowDepth, 32, 256,
                "maximum number of operator->s to follow")
 BENIGN_LANGOPT(InstantiationDepth, 32, 1024,

include/clang/Driver/CC1Options.td

 def static_define : Flag<["-"], "static-define">,
   HelpText<"Should __STATIC__ be defined">;
 def stack_protector : Separate<["-"], "stack-protector">,
   HelpText<"Enable stack protectors">;
 def stack_protector_buffer_size : Separate<["-"], "stack-protector-buffer-size">,
   HelpText<"Lower bound for a buffer to be considered for stack protection">;
 def fvisibility : Separate<["-"], "fvisibility">,
   HelpText<"Default type and symbol visibility">;
+def fvisibility_amdgpu_non_kernel_functions : Separate<["-"], "fvisibility-amdgpu-non-kernel-functions">,
+  HelpText<"Default non-kernel function symbol visibility for AMDGPU targets">;
 def ftype_visibility : Separate<["-"], "ftype-visibility">,
   HelpText<"Default type visibility">;
 def ftemplate_depth : Separate<["-"], "ftemplate-depth">,
   HelpText<"Maximum depth of recursive template instantiation">;
 def foperator_arrow_depth : Separate<["-"], "foperator-arrow-depth">,
   HelpText<"Maximum number of 'operator->'s to call for a member access">;
 def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">,
   HelpText<"Maximum depth of recursive constexpr function calls">;

include/clang/Driver/Options.td

 def fregister_global_dtors_with_atexit : Flag<["-"], "fregister-global-dtors-with-atexit">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use atexit or __cxa_atexit to register global destructors">;
 def fuse_init_array : Flag<["-"], "fuse-init-array">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use .init_array instead of .ctors">;
 def fno_var_tracking : Flag<["-"], "fno-var-tracking">, Group<clang_ignored_f_Group>;
 def fverbose_asm : Flag<["-"], "fverbose-asm">, Group<f_Group>;
 def fvisibility_EQ : Joined<["-"], "fvisibility=">, Group<f_Group>,
   HelpText<"Set the default symbol visibility for all global declarations">, Values<"hidden,default">;
+def fvisibility_amdgpu_non_kernel_functions_EQ : Joined<["-"], "fvisibility-amdgpu-non-kernel-functions=">, Group<f_Group>,
+  HelpText<"Set the default symbol visibility for non-kernel function declarations for AMDGPU targets">, Values<"hidden,default">;
 def fvisibility_inlines_hidden : Flag<["-"], "fvisibility-inlines-hidden">, Group<f_Group>,
   HelpText<"Give inline C++ member functions hidden visibility by default">,
   Flags<[CC1Option]>;
 def fvisibility_ms_compat : Flag<["-"], "fvisibility-ms-compat">, Group<f_Group>,
   HelpText<"Give global types 'default' visibility and global functions and "
            "variables 'hidden' visibility by default">;
 def fwhole_program_vtables : Flag<["-"], "fwhole-program-vtables">, Group<f_Group>,
   Flags<[CoreOption, CC1Option]>,

lib/CodeGen/TargetInfo.cpp

void AMDGPUTargetCodeGenInfo::setTargetAttributes(
   if (GV->isDeclaration())
     return;
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
   if (!FD)
     return;
 
   llvm::Function *F = cast<llvm::Function>(GV);
 
+  if (!FD->getExplicitVisibility(FunctionDecl::VisibilityForValue) &&
+      F->getCallingConv() != llvm::CallingConv::AMDGPU_KERNEL) {
+    GV->setVisibility(M.GetLLVMVisibility(
+        M.getLangOpts().getAMDGPUNonKernelFunctionVisibilityMode()));
+    M.setDSOLocal(GV);
+  }
+
   const auto *ReqdWGS = M.getLangOpts().OpenCL ?
     FD->getAttr<ReqdWorkGroupSizeAttr>() : nullptr;
 
   if (M.getLangOpts().OpenCL && FD->hasAttr<OpenCLKernelAttr>() &&
       (M.getTriple().getOS() == llvm::Triple::AMDHSA))
     F->addFnAttr("amdgpu-implicitarg-num-bytes", "48");
 
   const auto *FlatWGS = FD->getAttr<AMDGPUFlatWorkGroupSizeAttr>();

lib/Driver/ToolChains/AMDGPU.cpp

 }
 
 void AMDGPUToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
   // Default to "hidden" visibility, as object level linking will not be
   // supported for the forseeable future.
-  if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,
-                         options::OPT_fvisibility_ms_compat)) {
-    CC1Args.push_back("-fvisibility");
-    CC1Args.push_back("hidden");
+  if (!DriverArgs.hasArg(options::OPT_fvisibility_amdgpu_non_kernel_functions_EQ,
+                         options::OPT_fvisibility_amdgpu_non_kernel_functions)) {
+    CC1Args.append({"-fvisibility-amdgpu-non-kernel-functions", "hidden"});
   }
 }

lib/Driver/ToolChains/Clang.cpp

if (A->getOption().matches(options::OPT_fvisibility_EQ)) {
       assert(A->getOption().matches(options::OPT_fvisibility_ms_compat));
       CmdArgs.push_back("-fvisibility");
       CmdArgs.push_back("hidden");
       CmdArgs.push_back("-ftype-visibility");
       CmdArgs.push_back("default");
     }
   }
 
+  if (const Arg *A = Args.getLastArg(
+          options::OPT_fvisibility_amdgpu_non_kernel_functions_EQ)) {
+    CmdArgs.push_back("-fvisibility-amdgpu-non-kernel-functions");
+    CmdArgs.push_back(A->getValue());
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_fvisibility_inlines_hidden);
 
   Args.AddLastArg(CmdArgs, options::OPT_ftlsmodel_EQ);
 
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
   Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);

lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
 
   // The type-visibility mode defaults to the value-visibility mode.
   if (Arg *typeVisOpt = Args.getLastArg(OPT_ftype_visibility)) {
     Opts.setTypeVisibilityMode(parseVisibility(typeVisOpt, Args, Diags));
   } else {
     Opts.setTypeVisibilityMode(Opts.getValueVisibilityMode());
   }
 
+  // The amdgpu-non-kernel-function-visibility mode defaults to the
+  // value-visibility mode.
+  if (Arg *amdgpuVisOpt =
+          Args.getLastArg(OPT_fvisibility_amdgpu_non_kernel_functions)) {
+    Opts.setAMDGPUNonKernelFunctionVisibilityMode(
+        parseVisibility(amdgpuVisOpt, Args, Diags));
+  } else {
+    Opts.setAMDGPUNonKernelFunctionVisibilityMode(
+        Opts.getValueVisibilityMode());
+  }
+
   if (Args.hasArg(OPT_fvisibility_inlines_hidden))
     Opts.InlineVisibilityHidden = 1;
 
   if (Args.hasArg(OPT_ftrapv)) {
     Opts.setSignedOverflowBehavior(LangOptions::SOB_Trapping);
     // Set the handler, if one is specified.
     Opts.OverflowHandler =
         Args.getLastArgValue(OPT_ftrapv_handler);

test/CodeGen/visibility-amdgpu-non-kernel-functions.cl

+// RUN: %clang_cc1 -fvisibility hidden -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck --check-prefix=FVIS_HIDDEN %s
+// RUN: %clang_cc1 -fvisibility-amdgpu-non-kernel-functions hidden -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck --check-prefix=FVIS_AMDGPU_HIDDEN %s
+// RUN: %clang_cc1 -fvisibility-amdgpu-non-kernel-functions default -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck --check-prefix=FVIS_AMDGPU_DEFAULT %s
+// RUN: %clang_cc1 -fvisibility hidden -fvisibility-amdgpu-non-kernel-functions default -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck --check-prefix=FVIS_AMDGPU_OVERRIDE %s
+
+// FVIS_HIDDEN: define hidden amdgpu_kernel void @kern()
+// FVIS_AMDGPU_HIDDEN: define amdgpu_kernel void @kern()
+// FVIS_AMDGPU_DEFAULT: define amdgpu_kernel void @kern()
+// FVIS_AMDGPU_OVERRIDE: define hidden amdgpu_kernel void @kern()
+kernel void kern() {}
+// FVIS_HIDDEN: define amdgpu_kernel void @default_kern()
+// FVIS_AMDGPU_HIDDEN: define amdgpu_kernel void @default_kern()
+// FVIS_AMDGPU_DEFAULT: define amdgpu_kernel void @default_kern()
+// FVIS_AMDGPU_OVERRIDE: define amdgpu_kernel void @default_kern()
+__attribute__((visibility("default"))) kernel void default_kern() {}
+// FVIS_HIDDEN: define hidden amdgpu_kernel void @hidden_kern()
+// FVIS_AMDGPU_HIDDEN: define hidden amdgpu_kernel void @hidden_kern()
+// FVIS_AMDGPU_DEFAULT: define hidden amdgpu_kernel void @hidden_kern()
+// FVIS_AMDGPU_OVERRIDE: define hidden amdgpu_kernel void @hidden_kern()
+__attribute__((visibility("hidden"))) kernel void hidden_kern() {}
+// FVIS_HIDDEN: define protected amdgpu_kernel void @protected_kern()
+// FVIS_AMDGPU_HIDDEN: define protected amdgpu_kernel void @protected_kern()
+// FVIS_AMDGPU_DEFAULT: define protected amdgpu_kernel void @protected_kern()
+// FVIS_AMDGPU_OVERRIDE: define protected amdgpu_kernel void @protected_kern()
+__attribute__((visibility("protected"))) kernel void protected_kern() {}
+
+// FVIS_HIDDEN: define hidden void @func()
+// FVIS_AMDGPU_HIDDEN: define hidden void @func()
+// FVIS_AMDGPU_DEFAULT: define void @func()
+// FVIS_AMDGPU_OVERRIDE: define void @func()
+void func() {}
+// FVIS_HIDDEN: define void @default_func()
+// FVIS_AMDGPU_HIDDEN: define void @default_func()
+// FVIS_AMDGPU_DEFAULT: define void @default_func()
+// FVIS_AMDGPU_OVERRIDE: define void @default_func()
+__attribute__((visibility("default"))) void default_func() {}
+// FVIS_HIDDEN: define hidden void @hidden_func()
+// FVIS_AMDGPU_HIDDEN: define hidden void @hidden_func()
+// FVIS_AMDGPU_DEFAULT: define hidden void @hidden_func()
+// FVIS_AMDGPU_OVERRIDE: define hidden void @hidden_func()
+__attribute__((visibility("hidden"))) void hidden_func() {}
+// FVIS_HIDDEN: define protected void @protected_func()
+// FVIS_AMDGPU_HIDDEN: define protected void @protected_func()
+// FVIS_AMDGPU_DEFAULT: define protected void @protected_func()
+// FVIS_AMDGPU_OVERRIDE: define protected void @protected_func()
+__attribute__((visibility("protected"))) void protected_func() {}

test/Driver/amdgpu-visibility.cl

 // RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm %s 2>&1 | FileCheck -check-prefix=DEFAULT %s
-// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility=protected  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-PROTECTED  %s
-// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility-ms-compat  %s 2>&1 | FileCheck -check-prefix=OVERRIDE-MS  %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility=hidden %s 2>&1 | FileCheck -check-prefix=VISIBILITY-HIDDEN %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility-amdgpu-non-kernel-functions=hidden  %s 2>&1 | FileCheck -check-prefix=AMDGPU-HIDDEN  %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility-amdgpu-non-kernel-functions=default  %s 2>&1 | FileCheck -check-prefix=AMDGPU-DEFAULT  %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa -x cl -c -emit-llvm -fvisibility=hidden -fvisibility-amdgpu-non-kernel-functions=default  %s 2>&1 | FileCheck -check-prefix=VISIBILITY-HIDDEN-AMDGPU-DEFAULT  %s
 
-// DEFAULT: "-fvisibility" "hidden"
-// OVERRIDE-PROTECTED: "-fvisibility" "protected"
-// OVERRIDE-MS:  "-fvisibility" "hidden" "-ftype-visibility" "default"
+// DEFAULT: "-fvisibility-amdgpu-non-kernel-functions" "hidden"
+// VISIBILITY-HIDDEN: "-fvisibility-amdgpu-non-kernel-functions" "hidden"
+// AMDGPU-HIDDEN: "-fvisibility-amdgpu-non-kernel-functions" "hidden"
+// AMDGPU-DEFAULT: "-fvisibility-amdgpu-non-kernel-functions" "default"
+// VISIBILITY-HIDDEN-AMDGPU-DEFAULT: "-fvisibility-amdgpu-non-kernel-functions" "default"