Page MenuHomePhabricator

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

Authored by scott.linder on Oct 4 2018, 9:20 AM.

Details

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.

Diff Detail

Event Timeline

scott.linder created this revision.Oct 4 2018, 9:20 AM

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?

Update docs

arsenm added a comment.Oct 4 2018, 7:04 PM

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 added a comment.Oct 4 2018, 7:15 PM

Tests should also include some global variables

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 ?

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

arsenm added a comment.Oct 7 2018, 7:56 PM

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

t-tye added a comment.Oct 7 2018, 8:41 PM

Another word commonly used across languages is "offload".

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.

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.

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