[PATCH] D53424: Enable thread specific cl::opt values for multi-threaded support

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

May I ask why you think it's important to move away from `cl::opt` in the first place? What purpose does it actually solve?

I think having global `cl::opt` variables as descriptions of available options is a perfectly pragmatic thing to do. The storage of the chosen option values can be decoupled of the description of the options, which is something that this patch essentially does.

I mean, I get the idea that there are //certain kinds// of options which are associated to passes that you may run multiple times in a compilation flow, and you may want to be able to specify different option values for the different invocations of those passes; and yeah, those options are not served well by `cl::opt`. But many, perhaps most, options are not like this. Having them described as global variables is perfectly fine as far as I can tell.

Conversely, forcing such kinds of options into a new option struct has a clear disadvantage, which is that adding a new option (or possibly even modifying an existing one) will change a header file that will end up being included by essentially the whole world, so that adding a single option will force a recompile of large parts of the project, which is always a pain.

So from a pragmatic point of view, I think there is actually a strong reason for keeping `cl::opt`s around //as a means of describing available options//, even while decoupling storage for option values. I'm sure there's a fancy name to describe this kind of pattern, if that would help convince you of it. It's a really bad idea to start churn all over the code base unless you can actually name a clear benefit for that churn. The two items you've brought up so far are:

> - LLVM’s global options are great for easily changing things during development but they are terrible from a library user perspective. If the default values of these options are not good for a user it really suggests they need to be moved into a proper API.

I agree that LLVMParseCommandLine is not a great interface, but there's really nothing stopping us from adding a function `LLVMSetCommandLineOptionBool/Int/String(option_name, value)`. That's a perfectly fine API.

> - Given that `cl::opt`’s are usually global it feels extremely weird that reading and writing to these could actually be a thread local operation.

That's purely aesthetic opinion and not an argument at all.


Repository:
  rL LLVM

https://reviews.llvm.org/D53424