[PATCH] D89799: [clang][driver] Rename DriverOption as NoXarchOption (NFC)

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D89799#2345004 <https://reviews.llvm.org/D89799#2345004>, @awarzynski wrote:

> Thank you all for you comments! Please find my replies below. I've picked 4 main points raised here.
>
> 1
> -
>
> In D89799#2342677 <https://reviews.llvm.org/D89799#2342677>, @rnk wrote:
>
>> This seems like pretty corner case functionality. Do we really need this diagnostic?
>
> Good point. If nobody objects I will remove it.

+1 for removing it.

> 2
> -
>
> In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote:
>
>> I am not sure whether it is proper to rename it.
>>
>> Originally, this flag means driver option which is not supposed to be forwarded to tools. It is more like a reminder to driver developers since clang driver does not automatically forward options to tools and does not enforce not forwarding options with this flag to tools.
>
> I'm happy to rename this differently, e.g. `DontForwardToTools`. However, `DriverOption` is very confusing to me. Which driver is this flag referring to? Compiler driver (`clang`) ? Frontend driver (`clang -cc1`)? If it refers to libclangDriver in general, then everything in `Options.td` is a driver flag by default (i.e. it belongs to the library the implements the compiler driver API). Also, based on the current usage and feedback from [1]:
>
>> It served two purposes (1) (@awarzynski: No longer applies) (2) whether an option should be forwarded for -Xarch -Xarch_host -Xarch_device (macOS and CUDA use cases)
>
> I feel that `DriverOption` is currently too generic.

The original purposes have mostly been eliminated. The remaining is now -Xarch. `DontForwardToTools` does not sound useful to me. Very few tools (cc1/assembler/linker) accept driver options and they should just take an allowlist instead of requiring to annotate the opposite.

> 3
> -
>
> In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote:
>
>> Then some toolchains use this flag as a heuristic not to use options with this flag with -Xarch. For that purpose it is too broad as many options with this flag can be used with -Xarch.
>
> Isn't there a more broader problem with various flags being used incorrectly in Options.td? I would love to help improving that, but I'm new to this codebase. Are there any heuristics that I could use to guide me in that? For example - how do I decide where to delete `DriverOption` from?
> Btw, are you suggesting that this change will be incompatible with some downstream projects?
>
> 4
> -
>
> In D89799#2344672 <https://reviews.llvm.org/D89799#2344672>, @yaxunl wrote:
>
>> So the question is: Is there any value to keep the original intention of this flag, i.e. mark some options as driver options without enforcing it? Or do we want to add assertions or warnings in clang -cc1 to check if driver options are passed to FE?
>
> Isn't this already enforced in `ToolChain::TranslateXarchArgs` (via the diagnostic)? That's the only place where `DriverOption` is currently used.  Also, FE?
>
> Thank you for reviewing!
> [1] http://lists.llvm.org/pipermail/cfe-dev/2020-October/067023.html




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89799