This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Change default visibility to protected for device declarations
ClosedPublic

Authored by jhuber6 on Jan 20 2022, 10:09 AM.

Details

Summary

This patch changes the special-case handling of visibility when
compiling for an OpenMP target offloading device. This was orignally
added as a precaution against the bug encountered in PR41826 when
symbols in the device were being preempted by shared library symbols.
This should instead be done by making the visibility protected by default.
With protected visibility we are asserting that the symbols on the device
will never be preempted or preempt another symbol pending a shared library
load.

Diff Detail

Event Timeline

jhuber6 created this revision.Jan 20 2022, 10:09 AM
jhuber6 requested review of this revision.Jan 20 2022, 10:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 10:09 AM
jhuber6 updated this revision to Diff 401716.Jan 20 2022, 11:22 AM

Changing to use default protected instead of passing -Bsymbolic this should be more portable and make the intentions clearer.

jhuber6 retitled this revision from [OpenMP] Remove overriding visibility for device declarations to [OpenMP] Change default visibility to protected for device declarations.Jan 20 2022, 11:27 AM
jhuber6 edited the summary of this revision. (Show Details)
JonChesterfield accepted this revision.Jan 20 2022, 12:04 PM

Looks great, thanks!

This revision is now accepted and ready to land.Jan 20 2022, 12:04 PM

Does this still work with AMDGPU as we need it to?

clang/test/OpenMP/declare_target_codegen.cpp
293

What happened here?

jhuber6 added inline comments.Jan 20 2022, 12:20 PM
clang/test/OpenMP/declare_target_codegen.cpp
293

That was a special case I added that was only necessary when we were trying to remove things from being hidden. Now that hidden is not the default it's not necessary anymore.

jdoerfert added inline comments.Jan 20 2022, 12:34 PM
clang/test/OpenMP/declare_target_codegen.cpp
293

can we keep the test (coverage) anyway

jhuber6 updated this revision to Diff 401766.Jan 20 2022, 1:20 PM

Changing to use '-fvisibility=protected' when we construct the job. This is much more transparent and leaves the option open for the user to override it if they need default visibility.

clang/lib/Driver/ToolChains/Clang.cpp
5836

I think we need some more code here to detect if the user has already specified a value for fvisibility so that we don't clobber it

jhuber6 updated this revision to Diff 401811.Jan 20 2022, 4:01 PM

Forgot to make this mutually exclusive with user defined visibility value.

jhuber6 marked an inline comment as done.Jan 20 2022, 4:02 PM
jhuber6 added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5836

I put the code there with the intent of putting the else there, but forgot to actually put it there.

JonChesterfield accepted this revision.Jan 20 2022, 4:11 PM

LG, thanks!