[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Justin Bogner via Phabricator via cfe-commits]

bogner added a comment.

In D157151#4587522 , @awarzynski 
wrote:

> 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




Comment at: llvm/unittests/Option/OptionParsingTest.cpp:18
 
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+

awarzynski wrote:
> Why not just update the test?
I'd rather not remove the tests for the old API until we actually remove the API


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.
This revision is now accepted and ready to land.

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!




Comment at: clang/lib/Driver/Driver.cpp:1941-1945
+  // TODO: We're overriding the mask for flang here to keep this NFC for the
+  // option refactoring, but what we really need to do is annotate the flags
+  // that Flang uses.
   if (IsFlangMode())
+VisibilityMask = llvm::opt::Visibility(options::FlangOption);

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



Comment at: flang/docs/FlangDriver.md:248-250
+Options that are also supported by clang should explicitly specify `Default` in
+`Vis`, and options that are only supported in Flang should not specify
+`Default`.

For consistency



Comment at: llvm/unittests/Option/OptionParsingTest.cpp:18
 
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+

Why not just update the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Justin Bogner via Phabricator via cfe-commits]

bogner added a comment.

In D157151#4583904 , @awarzynski 
wrote:

> 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()) {
>   // vvv HERE vvv
>   // 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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D157151#4582441 , @bogner wrote:

> 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()) {
  // vvv HERE vvv
  // 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Justin Bogner via Phabricator via cfe-commits]

bogner added a comment.

In D157151#4582109 , @awarzynski 
wrote:

> 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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

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?




Comment at: clang/docs/InternalsManual.rst:669-670
+  can affect how the option is treated or displayed.
+* ``Vis`` may contain visibility-specific "tags" associated with the option.
+  This lets us filter options for specific tools.
 * ``Alias`` denotes that the option is an alias of another option. This may be

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`).



Comment at: clang/docs/InternalsManual.rst:682-685
+New options are recognized by the Clang driver if ``Vis`` is not specified or
+if it contains ``Default``. Options intended for the ``-cc1`` frontend must be
+explicitly marked with the ``CC1Option`` flag. Flags that specify ``CC1Option``
+but not ``Default`` will only be accessible via ``-cc1``.

"Clang driver" could mean two things:
* [[ 
https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/lib/Driver/CMakeLists.txt#L17-L102
 | clangDriver ]] - the driver library shared between Clang and Flang,
* [[ 
https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/tools/driver/CMakeLists.txt#L26-L36
 | 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.



Comment at: llvm/docs/ReleaseNotes.rst:160
+* The ``Flags`` field of ``llvm::opt::Option`` has been split into ``Flags``
+  and ``Visibility`` to simplify option sharing between clang's drivers.
+  Overloads of ``llvm::opt::OptTable`` that use ``FlagsToInclude`` have been

[nit] Worth clarifying and advertising a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Justin Bogner via Phabricator via cfe-commits]

bogner added a comment.

In D157151#4577790 , @awarzynski 
wrote:

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




Comment at: clang/include/clang/Driver/Options.h:25-37
-  CoreOption = (1 << 8),
-  CLOption = (1 << 9),
-  CC1Option = (1 << 10),
-  CC1AsOption = (1 << 11),
-  NoDriverOption = (1 << 12),
-  LinkOption = (1 << 13),
-  FlangOption = (1 << 14),

awarzynski wrote:
> What happens to `CoreOption`s? Same for `NoDriverOption`s?
- `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



Comment at: clang/include/clang/Driver/Options.td:193
 def m_x86_Features_Group : OptionGroup<"">,
-   Group, Flags<[CoreOption]>, DocName<"X86">;
+   Group, Vis<[Default, CLOption, DXCOption]>,
+   DocName<"X86">;

awarzynski wrote:
> What's `Default` in `Vis<[Default, CLOption, DXCOption]>,`?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

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?




Comment at: clang/include/clang/Driver/Options.h:25-37
-  CoreOption = (1 << 8),
-  CLOption = (1 << 9),
-  CC1Option = (1 << 10),
-  CC1AsOption = (1 << 11),
-  NoDriverOption = (1 << 12),
-  LinkOption = (1 << 13),
-  FlangOption = (1 << 14),

What happens to `CoreOption`s? Same for `NoDriverOption`s?



Comment at: clang/include/clang/Driver/Options.td:193
 def m_x86_Features_Group : OptionGroup<"">,
-   Group, Flags<[CoreOption]>, DocName<"X86">;
+   Group, Vis<[Default, CLOption, DXCOption]>,
+   DocName<"X86">;

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits