Page MenuHomePhabricator

[OpenMP] Remove hidden visibility for declare target variables
ClosedPublic

Authored by jhuber6 on Jan 14 2022, 2:23 PM.

Details

Summary

This patch changes the visiblity of variables declared within a declare
target directive. Variable declarations within a declare target
directive need to be externally visible to the plugin for initialization
or reading. Previously this would cause runtime errors where the named
global could not be found because it was not included in the symbol
table.

Diff Detail

Event Timeline

jhuber6 created this revision.Jan 14 2022, 2:23 PM
jhuber6 requested review of this revision.Jan 14 2022, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 2:23 PM
jhuber6 updated this revision to Diff 400292.Jan 15 2022, 8:31 AM

Changing method to use an early exit, previously visibility would still be hidden for some constructs.

jhuber6 updated this revision to Diff 400328.Jan 15 2022, 3:24 PM

Updating again, wasn't handling static variables declared in a record context
properly.

jdoerfert accepted this revision.Jan 18 2022, 7:58 AM

LG, let's hope this solves all the AMDGPU issues we've seen.

This revision is now accepted and ready to land.Jan 18 2022, 7:58 AM
This revision was landed with ongoing or failed builds.Jan 18 2022, 9:53 AM
This revision was automatically updated to reflect the committed changes.

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.

clang/lib/AST/Decl.cpp
792

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

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.

clang/lib/AST/Decl.cpp
792

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.

clang/lib/AST/Decl.cpp
1080

Possibly address space dependent. I'm not confident shared or thread local variables will behave sensibly if annotated with elf visibility flags.