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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm 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>,
----------------
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.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122558/new/

https://reviews.llvm.org/D122558