[PATCH] D122558: [2/3] TLS loads optimization (hoist)

[Eli Friedman via Phabricator via llvm-commits]

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