make better use of visibility attributes #49600
Conversation
| @@ -1197,11 +1204,13 @@ static void construct_vars(Module &M, Partition &partition) { | |||
| GlobalVariable::ExternalLinkage, | |||
| fidxs, "jl_fvar_idxs"); | |||
| fidxs_var->setVisibility(GlobalValue::HiddenVisibility); | |||
| fidxs_var->setDSOLocal(true); | |||
There was a problem hiding this comment.
I don't think we need to annotate these dso_local, they're removed by multiversioning later anyways.
| @@ -523,6 +525,7 @@ static GlobalVariable *emit_shard_table(Module &M, Type *T_size, Type *T_psize, | |||
| auto gv = new GlobalVariable(M, T_size, constant, | |||
| GlobalValue::ExternalLinkage, nullptr, name + suffix); | |||
| gv->setVisibility(GlobalValue::HiddenVisibility); | |||
| gv->setDSOLocal(true); | |||
There was a problem hiding this comment.
I don't think this is legal because the global is defined in a different shard (initializer is nullptr). Come to think of it, that hidden visibility marker may also not be legal to have.
There was a problem hiding this comment.
I am mostly just mechanically apply this to the HiddenVisibility values, since LLVM currently does that internally already, but may deprecate that in favor of expecting the front-end to specify both explicitly at the same time.
There was a problem hiding this comment.
The point of this annotation is that it instructs the compiler to assume the shard is simply a piece of large library. Otherwise it assumes it is a standalone unit and inhibits optimizations that need to see they will form a single unit later.
There was a problem hiding this comment.
I also would like to have this value, but a few lines down in dropUnusedDeclarations I needed to mark all the shard declarations as not dso_local because the linker would complain about not being able to fix up the relocations after compiling the system image. I don't know if that can become a problem here, but maybe it's worth a shot to try disabling that setting of not-dso_local?
There was a problem hiding this comment.
Oh, I see I missed quite a few more. This should hopefully work and correct some performance regressions I can see in the disassembly that were introduced by 6ab1862
| @@ -1556,6 +1565,7 @@ void jl_dump_native_impl(void *native_code, | |||
| GlobalVariable::ExternalLinkage, | |||
| gidxs, "jl_gvar_idxs"); | |||
| gidxs_var->setVisibility(GlobalValue::HiddenVisibility); | |||
| gidxs_var->setDSOLocal(true); | |||
There was a problem hiding this comment.
Same here, these variables are dropped by multiversioning.
vtjnash
force-pushed
the
jn/protect-visibility
branch
4 times, most recently
from
30ee681 to
5c7f78e
Compare
This pragma enables compilers to generate more optimal code than the identical command line flag, for better performance, by moving objects out of the GOT into direct references and eliminating the unnecessary PLT jump. Note that setting dllimport similarly enables more performance optimizations, at the cost of making duplicate symbols for functions so that they no longer have unique addresses (similar to the side-effect of setting -Bsymbolic-functions on ELF).
vtjnash
force-pushed
the
jn/protect-visibility
branch
from
9bc4903 to
8c8c339
Compare
| # ifdef LIBRARY_EXPORTS | ||
| # ifdef JL_LIBRARY_EXPORTS_INTERNAL | ||
| # define JL_DLLEXPORT __declspec(dllexport) | ||
| # else | ||
| # define JL_DLLEXPORT __declspec(dllimport) | ||
| # endif | ||
| # ifdef JL_LIBRARY_EXPORTS_CODEGEN | ||
| # define JL_DLLEXPORT_CODEGEN __declspec(dllexport) | ||
| # endif | ||
| #define JL_HIDDEN | ||
| #define JL_DLLIMPORT __declspec(dllimport) | ||
| #else | ||
| #define STDCALL | ||
| # define JL_DLLEXPORT __attribute__ ((visibility("default"))) | ||
| #define JL_DLLIMPORT | ||
| #define JL_DLLIMPORT __attribute__ ((visibility("default"))) | ||
| #define JL_HIDDEN __attribute__ ((visibility("hidden"))) | ||
| #endif | ||
| #ifndef JL_DLLEXPORT | ||
| # define JL_DLLEXPORT JL_DLLIMPORT | ||
| #endif | ||
| #ifndef JL_DLLEXPORT_CODEGEN | ||
| # define JL_DLLEXPORT_CODEGEN JL_DLLIMPORT |
There was a problem hiding this comment.
I really like this change since I ran exactly into the issues that the code doesn't seem to know which library exported what. Maybe with this change I can fix my current linkage issues trying to build julia with clang-cl (which requires correct __declspec(dllimport) statements to not run into linkage issues.).
This pragma enables compilers to generate more optimal code than the identical command line flag, for better performance, by moving objects out of the GOT into direct references.