[PATCH] D117362: [OpenMP] Remove hidden visibility for declare target variables

Wed Jan 19 07:12:36 PST 2022

jhuber6 added a subscriber: ronlieb.
jhuber6 added a comment.

In D117362#3254681 <https://reviews.llvm.org/D117362#3254681>, @JonChesterfield wrote:

> If I'm following correctly, this broke the amdgpu buildbot and it has been moved into staging as a workaround. I haven't debugged what breaks but 'DefaultVisibility' is relatively likely to behave differently on cuda vs hsa systems so that's my first guess.

Yes, this patch is necessary because of the different handling of `hidden` visibility by NVIDIA and AMDGPU. Without this patch we cannot run any code that uses `#pragma omp declare target` on AMDGPU because they are all hidden. The runtime will try to load it and fail so we will crash.

================
Comment at: clang/lib/AST/Decl.cpp:792
+    if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(Var))
+      return LinkageInfo::external();
+
----------------
JonChesterfield wrote:
> Would this change static variables to non-static and thus introduce multiple definition errors? Not immediately obvious to me that the variables have to be directly visible to other translation units
That was my first guess, I had @ronlieb run an alternate approach where we run everything as normal, but at the end we inherit the linkage but set the visibility to default. he said it still caused problems.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117362/new/

https://reviews.llvm.org/D117362