[PATCH] D122558: [2/3] TLS loads optimization (hoist)
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 28 18:06:58 PDT 2022
efriedma added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:3627
+ HelpText<"Hoist fetching TLS address in PIC model to remove redundant TLS address call">,
+ MarshallingInfoFlag<CodeGenOpts<"TlsAddrLoadHoist">>;
def mtls_direct_seg_refs : Flag<["-"], "mtls-direct-seg-refs">, Group<m_Group>,
----------------
xiangzhangllvm wrote:
> efriedma wrote:
> > clang driver options should generally have positive and negative versions (i.e. -mtls-load-hoist/-mno-tls-load-hoist).
> >
> > I'm not sure I understand why we need to expose this as a clang option, though. In the initial patch, you mentioned you wanted to land it off-by-default initially... but I'm not sure why we can't just flip the internal switch to turn it on by default, and call it done.
> First thanks for review!
> Let me add "-mno-tls-load-hoist" , can I ask a question ? (sorry for I am not much familiar with the clang driver) Why clang driver should have both positive and negative options ? (if it default to negative, doesn't only add positive is enough?)
>
> Yes, I land it off-by-default at [1/3] patch. Then I mentioned that:
> ```
> "This is a general optimization which will affect all targets test, I'd like to enable it by another independent patch [3/3] with mainly modify tests."
> ```
> Another purpose I want to just enable it in an independent patch is that I am not 100% sure it will has gain for all the targets. So I'd like to remain a easy way to revert the "enable patch" if necessary for some targets.
>
>
In general, providing flags both ways has two benefits:
1. It lets users request a specific behavior even if the default changes in the future.
2. Sometimes, in complex build system, editing flags is tricky, so it's helpful to be able to override a flag by just sticking the inverse flag at the end of the command-line.
---------
I think you're being too conservative in terms of the expected impact of this; it's hard for me to imagine a situation where hoisting a call to __tls_get_addr actually causes harm.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122558/new/
https://reviews.llvm.org/D122558
More information about the llvm-commits
mailing list