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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D122558#3415767 <https://reviews.llvm.org/D122558#3415767>, @xiangzhangllvm wrote:

> In D122558#3415640 <https://reviews.llvm.org/D122558#3415640>, @MaskRay wrote:
>
>> If the option is going to be on and never turned off, we don't need a clang driver option. Clang driver option has a commitment to relative stability.
>> For experimental things you can use `cl::opt`. `cl::opt` can be relatively freely removed.
>
> Make sense, I prefer to keep the clang driver option for customers
>  (in fact, this job is required from our customers and they point out they want an option to control it)
> Yes, I added `cl::opt` for it, PLS refer TLSVariableHoist.cpp: line 43.
> Thanks a lot for your reviewing!

This doesn't look like sufficient justification adding a new driver option for a feature which should just be enabled at all time.
Seems like the option is to work around potential issues just in case?
You can do that with `cl::opt`. In case anything bad happens, you can either revert the patch or tell the customer to add `-mllvm -xxx`.
When the feature seems robust enough, remove the `cl::opt` option.
Anyway, I just think we should not commit to have such a driver option.


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

https://reviews.llvm.org/D122558