Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented May 2, 2023

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.

@vtjnash vtjnash requested a review from gbaraldi May 2, 2023 18:33
GlobalVariable::ExternalLinkage,
fidxs, "jl_fvar_idxs");
fidxs_var->setVisibility(GlobalValue::HiddenVisibility);
fidxs_var->setDSOLocal(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to annotate these dso_local, they're removed by multiversioning later anyways.

auto gv = new GlobalVariable(M, T_size, constant,
GlobalValue::ExternalLinkage, nullptr, name + suffix);
gv->setVisibility(GlobalValue::HiddenVisibility);
gv->setDSOLocal(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GlobalVariable::ExternalLinkage,
gidxs, "jl_gvar_idxs");
gidxs_var->setVisibility(GlobalValue::HiddenVisibility);
gidxs_var->setDSOLocal(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, these variables are dropped by multiversioning.

@vtjnash vtjnash force-pushed the jn/protect-visibility branch 4 times, most recently from 30ee681 to 5c7f78e Compare May 8, 2023 16:06
@gbaraldi gbaraldi added the merge me PR is reviewed. Merge when all tests are passing label May 8, 2023
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 vtjnash force-pushed the jn/protect-visibility branch from 9bc4903 to 8c8c339 Compare May 9, 2023 13:13
@vtjnash vtjnash merged commit 056112e into master May 10, 2023
@vtjnash vtjnash deleted the jn/protect-visibility branch May 10, 2023 16:54
Comment on lines -75 to +92
# 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.).

@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants