Page MenuHomePhabricator

[clang][driver] Rename DriverOption as NoXarchOption (NFC)
ClosedPublic

Authored by awarzynski on Oct 20 2020, 8:43 AM.

Details

Summary

As discussed in [1], ClangFlags::DriverOption is currently only used to
mark options that should not be forwarded to other tools via -Xarch
options. This patch renames this flag accordingly and updates the
corresponding driver diagnostic.

A comment in ToolChain::TranslateXarchArgs is also updated to reflect
the change. The original comment referred to isDriverOption(), which is
no longer available.

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html

Diff Detail

Event Timeline

awarzynski created this revision.Oct 20 2020, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 8:43 AM
awarzynski requested review of this revision.Oct 20 2020, 8:43 AM
jdoerfert resigned from this revision.Oct 20 2020, 9:15 AM
awarzynski edited reviewers, added: MaskRay, tra, rnk, hans; removed: jdoerfert.Oct 20 2020, 12:51 PM

LGTM. I think a number of options can probably drop the flag, but that can be left as a future clean-up for macOS -Xarch (can someone add a reviewer for macOS?) and CUDA -Xarch* folks.

rnk added a comment.Oct 20 2020, 1:37 PM

This seems like pretty corner case functionality. Do we really need this diagnostic? @tra @yaxun

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.

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.

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?

Thank you all for you comments! Please find my replies below. I've picked 4 main points raised here.

1

In 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.

2

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.

3

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

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

Thank you all for you comments! Please find my replies below. I've picked 4 main points raised here.

1

In 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

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

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

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

The original purposes have mostly been eliminated. The remaining is now -Xarch.

OK. Then let's rename it.

Remove the diagnostic

  • Deleted the err_drv_invalid_Xarch_argument_unsupported diagnostic
  • Removed const from &getDiags() - otherwise I couldn't use getCustomDiagID (which is a non-const member function). DiagnosticEngine is a non-const member variable of clang::driver::Driver, so this change should be fine.
MaskRay accepted this revision.Thu, Oct 29, 9:59 AM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
1234

If this is not of much uses, hope we can remove it in the future.

This revision is now accepted and ready to land.Thu, Oct 29, 9:59 AM
This revision was landed with ongoing or failed builds.Fri, Oct 30, 10:01 AM
This revision was automatically updated to reflect the committed changes.