[PATCH] D105881: [flang][driver] Switch to `BoolFOption` for boolean options

Tue Jul 27 04:49:58 PDT 2021

awarzynski added a comment.

Thank you for your detailed reply!

In D105881#2904699 <https://reviews.llvm.org/D105881#2904699>, @jansvoboda11 wrote:

> Sorry, I'm not sure I follow.

OK, let me try to clarify. I've tried to split this into threads.

(flag == OptionFlag <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/llvm/include/llvm/Option/OptParser.td#L54> )

**SEMANTICS**

> In D105881#2890134 <https://reviews.llvm.org/D105881#2890134>, @awarzynski wrote:
>
>> @jansvoboda11 , now I'm realising the key disadvantage of using `OptInFFlag/OptOutFFlag` - it's impossible to express the `opt-in`/`opt-out` semantics in TableGen.
>
> What do you mean exactly?

BoolOption <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/clang/include/clang/Driver/Options.td#L399-L425> (from which BoolFOption <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/clang/include/clang/Driver/Options.td#L431-L436> derives) has e.g. the `default` value and a bunch of `assert`s to define the relation between `-ffoo` and `-fno-foo`. So effectively, the opt-in/opt-out semantics are enforced here. `OptInFFlag`/`OptOutFFlag` have none of this. It's just a syntactic sugar to automate the generation of `-ffoo` and `-fno-foo`. That's fine, we can add such checks inside the driver.

**CC1Option**

> I think the semantics are expressed pretty well from Clang's point of view:

OK, that's fair enough. But what about the output from `clang --help` and `clang -cc1 --help`? Let's look at -feliminate-unused-debug-types <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/clang/include/clang/Driver/Options.td#L1392-L1393> (the only instance of `OptOutFFLag`):

  # Only -fno-eliminate-unusued-debug-types is printed
  clang -cc1 --help | grep eliminate-unused
    -fno-eliminate-unused-debug-types
  # Both -fno-eliminate-unusued-debug-types and -feliminate-unusued-debug-types printed?
  clang --help | grep eliminate-unused
    -feliminate-unused-debug-types
    -fno-eliminate-unused-debug-types

To me this behavior is counter-intuitive. So are these `clang` options? `clang -cc1` options? Both? What's the point of adding `CC1Option` if both `clang` and `clang -cc1` accept them. The definition of `CC1Option` from Options.td:

  // CC1Option - This option should be accepted by clang -cc1.
  def CC1Option : OptionFlag;

OK, so currently `-fno-eliminate-unused-debug-types` is a `CC1Option` and _should_ be accepted by `clang -cc1`. How about `-feliminate-unused-debug-types`?

  clang -cc1 -feliminate-unused-debug-types file.c
  error: unknown argument: '-feliminate-unused-debug-types'

Makes sense, but:

  clang -c -feliminate-unused-debug-types file.c
  clang-13: warning: argument unused during compilation: '-feliminate-unused-debug-types' [-Wunused-command-line-argument]

Shouldn't it be rejected by `clang` with an error as well? Shouldn't `CC1Option` be used consistently for `-feliminate-unused-debug-types`  and `-fno-eliminate-unused-debug-types`?

>> In fact, only the contents of `clang -cc1 --help` are being tweaked by using `OptInFFlag/OptOutFFlag`.
>
> That's incorrect, `OptInFFlag` and `OptOutFFlag` both set `HelpText` for the positive and negative driver flags.

Yes, but in `clang --cc1 --help`, only the variant with `CC1Option` is being printed and the other one is ignored. So why bother adding the other one?

Btw, now I see that `-cc1` does reject `-feliminate-unused-debug-types`, so it's not only about the contents of `clang -cc1 --help`. I was wrong, sorry about that and thanks for pointing this out!

**Content of `--help`**

>> In Flang, we rely on the fact that "no help text? don't include it in `flang-new -fc1 --help`" instead. This behavior is implemented in `clangDriver` and applies to all drivers that use it (so this is nothing specific to Flang).
>
> I see. Is this intentional?
>
>> This way, we can always mark Flang options with the relevant flags (e.g. `FC1Option`). In other words, we can claim them and make it clear that other drivers can ignore them.
>
> I don't understand this. Can you elaborate? Are you relying on the absence/presence of a help text to mark flags with `FC1Option`? When does this happen? (In `Options.td`? The TableGen backend? The consumer of `Options.inc`?)

See the implementation of printHelp <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/llvm/lib/Option/OptTable.cpp#L627-L671> from OptTable.cpp. Basically: "No help text? Not printing". We rely on this **not to print** options supported by Flang, but for which we have no help text. We only used that for boolean options (so we have a help text for e.g. `-ffoo`, but not for `-fno-foo`). This is a minor thing and I am not concerned about loosing it.

As for `FC1Option` (and `FlangOption`), we always add these explicitly in Options.td. This helps to identify options that are either shared or Flang-only and to avoid polluting `clang --help` with Flang/Fortran specific options. And that's what we rely on when calling printHelp <https://github.com/llvm/llvm-project/blob/2e27c4e1f187446c84220f75e492f16807d21b12/flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp#L106-L110>.

> I'd need to double-check if we particularly care about showing/hiding flags without help text.

We should. `clang --help` should not be printing any Flang options, and vice-versa. I'm under the impression that the contents of `clang --help` and `clang --cc1 --help` got a bit out of control. We did discuss this briefly on cfe-dev <https://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html>. I guess that with ~1000 options supported by Clang, this is rarely noticed. Does anyone use `clang --help` these days? But in Flang, we have only ~50 options and if we are not careful, `flang --help` would be printing hundreds of options that are not supported (and in fact, will never be).

We try to be strict about what is and what is not included in `flang --help` as that's currently our main end-user-facing documentation. We do this by adding a Flang flag (e.g. `FlangOption` or `FC1Option`) to every option that we support. Options that don't contain flags (e.g. `feliminate-unused-debug-types`) are very problematic for us. What is Flang meant to do with such options? This is an issue for every tool that relies on Options.td. This also works the other way - if we don't flag Flang options, `clang --help` and `clang -cc1 --help` will start printing options added by us.  Same mechanism applies to auto-completion for command line options or for finding suggestions when an option is misspelled.

**Splitting Options.td**

>> In general, I think that Options.td would become a bit cleaner and easier to reason about if all options specified all their flags. That wouldn't be easy to achieve though!
>
> I think that the way we define flags for the Clang driver and `-cc1` in one go is already bit confusing. But it's convenient, so we roll with it.

I find it very hard to reason about things in Options.td. IMHO, we should have a separate Options.td for generic driver options (shared between Clang and Flang), and then separate Options.td for `cc1` and `fc1`. This was brought up when we discussed implementing the Flang driver in terms of `clangDriver`. It would help in forcing every driver to clearly define what options it supports. Also, the long term goal is to move `clangDriver` out of Clang, at which point that's going to be required anyway.

IMO, `OptInFFlang`/`OptOutFFlag` are generic enough and should be shared.

**Next steps**

>> It would be nicer to have the `opt-in`/`opt-out` semantics for Flang in TableGen through something similar to `BoolFOption`. No need for this just now - one step at a time :)
>
> I suggest to find another means of achieving the semantics you want. You can always copy the `BoolFOption` implementation and remove the marshalling stuff, if that's really the semantics you want.

The updated `OptInFFlang`/`OptOutFFlag` work just fine for us.  Flang does not need `EmptyKPM` and `BoolFOption` just now. I'm not suggesting any changes here.

As there's only a handful of options using `OptInFFlang`/`OptOutFFlag` right now, I was hoping that the current revision would be non-controversial. We definitely need to use `FC1Option` (or other Flang flag) for both `-ffoo` and `-fno-foo`. Otherwise, `clang --help` will start printing/supporting the version without a flag. If adding a flag to both variants is not acceptable in `-cc1`, then I suggest splitting `OptInFFlag`/`OptOutFFlag` into

- `OptInCC1FFlang`/`OptOutCC1FFlag`, and
- `OptInFC1FFlang`/`OptOutFC1FFlag`.

As for the help text, what would be best for Clang and `-cc1`? Add it to both `-ffoo` and `-fno-foo`? Just one of them?

Thanks for reviewing this and all your comments! I hope that we can figure something out that will work for both Clang and Flang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105881