This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Refactor to use llvm Option's new Visibility flags
ClosedPublic

Authored by bogner on Aug 4 2023, 3:53 PM.

Details

Summary

This is a big refactor of the clang driver's option handling to use
the Visibility flags introduced in https://reviews.llvm.org/D157149.
There are a few distinct parts, but they can't really be split into
separate commits and still be made to compile.

  1. We split out some of the flags in ClangFlags to ClangVisibility. Note that this does not include any subtractive flags.
  1. We update the Flag definitions and OptIn/OptOut constructs in Options.td by hand.
  1. We introduce and use a script, update_options_td_flags, to ease migration of flag definitions in Options.td, and we run that on Options.td. I intend to remove this later, but I'm committing it so that downstream forks can use the script to simplify merging.
  1. We update calls to OptTable in the clang driver, cc1as, flang, and clangd to use the visibility APIs instead of Include/Exclude flags.
  1. We deprecate the Include/Exclude APIs and add a release note.

*if you are running into conflicts with this change:*

Note that https://reviews.llvm.org/D157150 may also be the culprit and
if so it should be handled first.

The script in clang/utils/update_options_td_flags.py can help. Take
the downstream side of all conflicts and then run the following:

% cd clang/include/clang/Driver
% ../../../utils/update_options_td_flags.py Options.td > Options.td.new
% mv Options.td.new Options.td

This will hopefully be sufficient, please take a look at the diff.

Diff Detail

Event Timeline

bogner created this revision.Aug 4 2023, 3:53 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
bogner requested review of this revision.Aug 4 2023, 3:53 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
bogner updated this revision to Diff 548834.Aug 9 2023, 5:40 PM

Rebase/resolve conflicts

bogner updated this revision to Diff 549110.Aug 10 2023, 11:03 AM

Resolve conflicts

Hey @bogner , I've only skimmed through so far and it's looking great! That Include/Exclude API was not fun to use. What you are proposing here takes Options.td to a much a better place - this is a much needed and long overdue refactor.

There's quite a bit of churn here, so I will need a few days to scan. In the meantime, could you update flang/docs/FlangDriver.md? And in general, please note that this updates (primarily) clangDriver logic, which is used by both by Clang and Flang. In particular, Options.td is shared. I think that it's worth highlighting that this change benefits both sub-projects.

introduced in llvm Option

Could you add a link?

clang/include/clang/Driver/Options.h
26–38

What happens to CoreOptions? Same for NoDriverOptions?

clang/include/clang/Driver/Options.td
198–199

What's Default in Vis<[Default, CLOption, DXCOption]>,?

bogner edited the summary of this revision. (Show Details)Aug 10 2023, 3:16 PM

Hey @bogner , I've only skimmed through so far and it's looking great! That Include/Exclude API was not fun to use. What you are proposing here takes Options.td to a much a better place - this is a much needed and long overdue refactor.

There's quite a bit of churn here, so I will need a few days to scan. In the meantime, could you update flang/docs/FlangDriver.md? And in general, please note that this updates (primarily) clangDriver logic, which is used by both by Clang and Flang. In particular, Options.td is shared. I think that it's worth highlighting that this change benefits both sub-projects.

Yep, sorry about that. I tried to break this up as much as I could but given the number of places that depend on Options.td it was tricky. I do think this should makes things much easier for flang's driver in particular, as well as making clang-cl and clang-dxc easier to deal with.

Sorry I missed updating FlangDriver.md - FWIW I did build flang and run those tests with this change. I'll update the patch with doc updates momentarily.

introduced in llvm Option

Could you add a link?

I've updated the description to mention https://reviews.llvm.org/D157149.

clang/include/clang/Driver/Options.h
26–38
  • CoreOption meant "available in clang and clang-cl (and maybe dxc...)", so it's now [Default, CLOption].
  • NoDriverOption is !Default - negative flags were a big part of why the include/exclude thing was awkward, so the change in https://reviews.llvm.org/D157149 introduced a bit for options where you don't say anything specific, which meant the clang driver in practice.

For a complete map, see flag_to_vis in the update_options_td_flags.py helper script I provided for downstreams: https://reviews.llvm.org/D157151#change-FMUYy4yRDQLQ

clang/include/clang/Driver/Options.td
198–199

If you don't specify any visibility, the visibility is "Default" (See https://reviews.llvm.org/D157149). For all intents and purposes this means "Clang Driver" in this file.

bogner updated this revision to Diff 549188.Aug 10 2023, 3:40 PM

Update FlangDriver.md

I've tested this locally and can confirm that the behavior of clang and flang-new has been preserved (as in, these changes won't be visible to the end users). Nice!

I think that it would be good to replace Default with e.g.

  • Clang, or
  • ClangDriver, or
  • ClangCompilerDriver.

Or, at least, to make the meaning of Default much clearer (through e.g. comments). In general, I feel that Default is skewed towards this notion that clang (the compiler driver tool) is the main client of clangDriver. That used to be the case, but with Flang's driver also implemented in terms of clangDriver , that's no longer the case. Also, the long term goal is to extract the driver library out of Clang. One day :)

Also, note that Default options will be available in both clang and flang-new. That's the case today (so not something affected by your changes). Ideally, Flang should be able to disable those _Clang options_. That's not possible ATM. Contrary to Flang, Clang can disable _Flang options_ with FlangOnlyOption. There is no such flag for Flang ATM, but I think that we could re-use Default for that. WDYT?

clang/docs/InternalsManual.rst
667–668

IMHO, the difference between Flags and Vis is still unclear. Lets take this opportunity to refine this. IIUC:

  • vis should be used to specify the drivers in which a particular option would be available. This attribute will impact tool --help,
  • flags can be used to limit the contexts in which a particular option/flag can be used (e.g. NoXarchOption or LinkerOption).
682–687

"Clang driver" could mean two things:

  • clangDriver - the driver library shared between Clang and Flang,
  • clang - Clang's compiler driver implemented in terms of clangDriver library.

I think that it's important to distinguish between the two in this document. In particular, the meaning of Default is confusing. Is the the default for the library (clangDriver) or the Clang compiler driver binary, clang. I believe it's the latter, but this wording suggests the former.

I appreciate that this wording pre-dates your patch (and probably Flang), but I think that it's worth taking this opportunity to refine this.

llvm/docs/ReleaseNotes.rst
162

[nit] Worth clarifying and advertising a bit more.

I think that it would be good to replace Default with e.g.

  • Clang, or
  • ClangDriver, or
  • ClangCompilerDriver.

Or, at least, to make the meaning of Default much clearer (through e.g. comments). In general, I feel that Default is skewed towards this notion that clang (the compiler driver tool) is the main client of clangDriver. That used to be the case, but with Flang's driver also implemented in terms of clangDriver , that's no longer the case. Also, the long term goal is to extract the driver library out of Clang. One day :)

This is a little bit complicated by the fact that the Option library is already partially extracted out of clang (and used for other drivers, like lld and lldb). The "Default" visibility is defined in llvm's Option library, so calling it something like "Clang" would be pretty confusing for users that aren't the clang Driver library. I suppose one option would be to throw something like using ClangDriver = llvm::Driver::Default; in Options.h inside the clang::driver::options namespace, and then using the alias in Options.td. Would that help?

Also, note that Default options will be available in both clang and flang-new. That's the case today (so not something affected by your changes). Ideally, Flang should be able to disable those _Clang options_. That's not possible ATM. Contrary to Flang, Clang can disable _Flang options_ with FlangOnlyOption. There is no such flag for Flang ATM, but I think that we could re-use Default for that. WDYT?

Default options are either options that don't specify Vis at all or explicitly mention Default. They are not visible in flang-new after this change unless they also specify FlangOption. Specifically, instead of having to deal with FlangOnlyOption we explicitly opt in to default. Some examples:

def default : Flag<["-"], "a">; //visible only in invocations of `clang`
def default_explicit : Flag<["-"], "b">, Vis<[Default]>; //visible only in invocations of `clang`
def flang_only : Flag<["-"], "c">, Vis<[FlangOption]>; // only `flang-new`
def flang_and_clang : Flag<["-"], "d">, Vis<[Default, FlangOption]>; // Both `clang` and `flang-new`
def flang_fc1 : Flag<["-"], "e">, Vis<[FC1Option]>; // `flang-new -fc1`, but not the `flang-new` driver or `clang`

This is a little bit complicated by the fact that the Option library is already partially extracted out of clang (and used for other drivers, like lld and lldb). The "Default" visibility is defined in llvm's Option library, so calling it something like "Clang" would be pretty confusing for users that aren't the clang Driver library.

Ah, I see. I guess clangDriver is a bit unique in the sense that there's quite a few drivers that rely on its Options.td:

  • clang, clang -cc1, clang -cc1as, clang-cl,
  • flang-new, flang-new -fc1.

I suppose one option would be to throw something like using ClangDriver = llvm::Driver::Default; in Options.h inside the clang::driver::options namespace, and then using the alias in Options.td. Would that help?

That would make sense to me, yes. Otherwise the meaning of Default is unclear. These things tend to be obvious to folks familiar with the implementation. However, Options.td would normally be edited/updated by people less familiar with the fine details and more concerned about their favorite option and how it's implemented in their favorite tool.

Default options are either options that don't specify Vis at all or explicitly mention Default. They are not visible in flang-new after this change unless they also specify FlangOption.

That's not quite true ATM. Although not used, the following C++ option _is visible_ in flang-new:

flang-new -fno-experimental-relative-c++-abi-vtables file.f90
flang-new: warning: argument unused during compilation: '-fno-experimental-relative-c++-abi-vtables'

This can be tweaked in getOptionVisibilityMask (extracted from this patch):

llvm::opt::Visibility
Driver::getOptionVisibilityMask(bool UseDriverMode) const {
  if (!UseDriverMode)
    return llvm::opt::Visibility(llvm::opt::Default);
  if (IsCLMode())
    return llvm::opt::Visibility(options::CLOption);
  if (IsDXCMode())
    return llvm::opt::Visibility(options::DXCOption);
  if (IsFlangMode()) {
    // vvvvvvvvvvvvvvv HERE vvvvvvvvvvvvvvvvvvv
    // TODO: Does flang really want *all* of the clang driver options?
    // We probably need to annotate more specifically.
    return llvm::opt::Visibility(llvm::opt::Default | options::FlangOption);
  }
  return llvm::opt::Visibility(llvm::opt::Default);
}

Now, prior to this change I couldn't find a way to disable unsupported Clang options in Flang. With this patch,

  • all Clang options gain a visibility flag (atm that's Default),
  • that flag can be used to disable options unsupported in Flang.

For this to work, all options supported by Flang need to have their visibility flags set accordingly. That's the goal, but I can see that we have missed quite a few options over the time (*). Updated here: https://reviews.llvm.org/D157837.

(*) There was no mechanism to enforce that. This is now changing.

bogner updated this revision to Diff 549865.Aug 14 2023, 3:34 AM
bogner marked 3 inline comments as done.

Update to use "ClangOption" instead of "Default" (See https://reviews.llvm.org/D157150 for the introduction of the name), and address documentation comments.

This can be tweaked in getOptionVisibilityMask (extracted from this patch):

llvm::opt::Visibility
Driver::getOptionVisibilityMask(bool UseDriverMode) const {
  if (!UseDriverMode)
    return llvm::opt::Visibility(llvm::opt::Default);
  if (IsCLMode())
    return llvm::opt::Visibility(options::CLOption);
  if (IsDXCMode())
    return llvm::opt::Visibility(options::DXCOption);
  if (IsFlangMode()) {
    // vvvvvvvvvvvvvvv HERE vvvvvvvvvvvvvvvvvvv
    // TODO: Does flang really want *all* of the clang driver options?
    // We probably need to annotate more specifically.
    return llvm::opt::Visibility(llvm::opt::Default | options::FlangOption);
  }
  return llvm::opt::Visibility(llvm::opt::Default);
}

Now, prior to this change I couldn't find a way to disable unsupported Clang options in Flang. With this patch,

  • all Clang options gain a visibility flag (atm that's Default),
  • that flag can be used to disable options unsupported in Flang.

For this to work, all options supported by Flang need to have their visibility flags set accordingly. That's the goal, but I can see that we have missed quite a few options over the time (*). Updated here: https://reviews.llvm.org/D157837.

Ah right. Yes, this is exactly the problem that this patch is trying to fix. Glad to see that it seems to be working as intended for flang-new!

bogner updated this revision to Diff 550126.Aug 14 2023, 4:29 PM

Rebased and finished the changes to use ClangOption everywhere instead of DefaultVis. I think this is good to go now, please take a look.

awarzynski accepted this revision.EditedAug 15 2023, 1:33 AM

While quite extensive, I find the overall logic in this patch very well structured and executed in a very clean manner. It removes a lot of ambiguity, makes the overall design much easier to reason about and will definitely improve the overall health of the project. The benefits for LLVM Flang are almost immediate, see https://reviews.llvm.org/D157837. Thank you for implementing this, reviewing such patches is very satisfying and rewarding.

This might cause some disruption to downstream consumers of this API and Options.td. Hopefully, "update_options_td_flags.py" that you've included should minimise that. I suggest "advertising" it in the summary a bit more.

LGTM, great work, thank you!

EDIT When landing this, could you send a PSA to Discourse to make folks aware of what's coming? I am primarily concerned about our downstream users.

clang/lib/Driver/Driver.cpp
1941–1945

Note to myself - I should update this in https://reviews.llvm.org/D157837.

flang/docs/FlangDriver.md
248–250

For consistency

llvm/unittests/Option/OptionParsingTest.cpp
18

Why not just update the test?

This revision is now accepted and ready to land.Aug 15 2023, 1:33 AM

This might cause some disruption to downstream consumers of this API and Options.td. Hopefully, "update_options_td_flags.py" that you've included should minimise that. I suggest "advertising" it in the summary a bit more.

LGTM, great work, thank you!

When landing this, could you send a PSA to Discourse to make folks aware of what's coming? I am primarily concerned about our downstream users.

Good call, and that should help a bit with advertising what to do if a downstream breaks. I'll do so

llvm/unittests/Option/OptionParsingTest.cpp
18

I'd rather not remove the tests for the old API until we actually remove the API

bogner updated this revision to Diff 550225.Aug 15 2023, 2:36 AM

Rebase for Vis -> Visibility change in https://reviews.llvm.org/D157149, and a few fixes for the Default -> ClangOption change I missed on my last update.

bogner edited the summary of this revision. (Show Details)Aug 15 2023, 11:56 AM
bogner updated this revision to Diff 550418.Aug 15 2023, 11:59 AM

Update release note to point at the reviews for conflict resolution instructions.

This revision was landed with ongoing or failed builds.Aug 15 2023, 2:27 PM
This revision was automatically updated to reflect the committed changes.
bogner marked an inline comment as done.Aug 15 2023, 4:27 PM
nikic added a subscriber: nikic.Aug 15 2023, 11:40 PM

To avoid a full revert, I removed the deprecations in this patch in https://github.com/llvm/llvm-project/commit/348d36f1a2c8c776ce87e038bf15fc4cabd62702. Please do not deprecate methods that have remaining in-tree users and make sure that -Werror=deprecated-declarations builds work.