[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

Mon Nov 30 17:11:40 PST 2020

dexonsmith added a comment.

In D83892#2417903 <https://reviews.llvm.org/D83892#2417903>, @jansvoboda11 wrote:

> This is now ready to be reviewed. If you see a deleted option in the diff, it's either because it was moved closer to its counterpart, or because it's now generated by a multiclass.
> I've added a bunch of TODOs I plan to address in a future patch, most of them aim to compress two options into a single multiclass that describes their relationship (e.g. `OptOutPositiveFlag`).
> I'd like to make `OptInFFlag` et al. modular with something like `OptInFlag` that does not imply `Flags<[CC1Option]>` and `Group<f_Group>` and can be marked with `IsPositive` or `IsNegative`.

I think it will be easier to review if we work out the "right" scheme, land prep patches so the existing options match that scheme, and then land the CodeGen options with whatever additional functionality is useful. I think there are really two things to decide on for the scheme: what are the axes, and how are they represented by multiclass?

For the axes, I think this patch changes them to:

- matching of command-line+storage polarity: do the command-line and the storage have the same polarity? ("opt-in" means: "default==false" == "-fno-flag is implied").
- default storage value: does the command-line default correspond to `false` or `true`?

I find this a bit hard to reason about. I feel like "opt-in" vs. "opt-out" should refer purely to command-line option behaviour, independently of storage implementation. Then the second axis can map that somehow to storage / marshalling.

IIUC, the previous / in-tree axes are:

- command-line behaviour: is "flag" opt-in or opt-out in `-cc1`? (opt-in implies `-fno-flag`, opt-out implies `-fflag`)
- default storage value: does the command-line default correspond to `false` or `true`? (I think "Positive" means "default is false"?)

For me, that's a bit easier to reason about since the axes seem quite independent. Another option (I'm not sure it's as good) is:

- command-line behaviour: is "flag" opt-in or opt-out in `-cc1`? (opt-in implies `-fno-flag`, opt-out implies `-fflag`)
- flag storage value: does "flag" being on correspond to `true` or `false`?

For the second axis, I don't find the existing "positive" vs. "negative" intuitive. Maybe it could be `DefaultStoredAsFalse` vs. `DefaultStoredAsTrue` (or, if we changed the axis, `OnMeansTrue` vs `OnMeansFalse`) or maybe there are better/shorter names.

WDYT?

For the multiclass, I don't have a strong opinion, as long as it's declarative and clear. One possibility is to enumerate `Out{Out,In}FFlagDefaultStoredAs{False,True}` (or `Out{Out,In}FFlag{,DefaultStoredAsTrue}`); I think this would be fine. It's also fine to take the axes as arguments in a single multiclass, or have `Opt{Out,In}FFlag` and take the second axis as an argument.

================
Comment at: clang/include/clang/Driver/Options.td:250
+// (FastMath == false) and enabled by -ffast-math (FastMath == true).
+// todo: rename to OptInPositiveFFlag
 multiclass OptInFFlag<string name, string pos_prefix, string neg_prefix="",
----------------
Whatever names we go with, I think it'll be cleaner to rename existing multiclasses first, and then add in the new multiclasses and/or functionality. That minimizes how inconsistency in-tree (compared to landing new options with a different naming scheme, and then fixing the old options as a follow-up).

================
Comment at: clang/include/clang/Driver/Options.td:281-283
+// This is useful if the option has negative semantics (e.g. `NoUseJumpTables`) that
+// is by default disabled (NoUseJumpTables == false), can be reaffirmed by -fuse-jump-tables
+// (NoUseJumpTables == false) and enabled by -fno-use-jump-tables (NoUseJumpTables == true).
----------------
I find the terminology here unintuitive. the thing being declared is when using this multiclass is a command-line option, so I feel like the word "option" should refer to `-fuse-jump-tables`, and `NoUseJumpTables` is a storage location / keypath for the option. In that case, `-fuse-jump-tables` is enabled by default, can be disabled by `-fno-use-jump-tables`, and this default state is stored in `NoUseJumpTables` with the value `false`.

================
Comment at: clang/include/clang/Driver/Options.td:284-285
+// (NoUseJumpTables == false) and enabled by -fno-use-jump-tables (NoUseJumpTables == true).
+// todo: rename to OptInNegativeFFlag
 multiclass OptOutFFlag<string name, string pos_prefix, string neg_prefix,
                        string help="", list<OptionFlag> flags=[], code keypath="",
----------------
I'm finding the suggested rename a bit confusing; per my inline comment above, this seems like it should be `OptOut...FFlag`, since (per my terminology) the command-line option `-fuse-jump-tables` is enabled by default.

================
Comment at: clang/include/clang/Driver/Options.td:293
                MarshallingInfoFlag<keypath, "false">,
                ImpliedByAnyOf<disablers, "true">;
 }
----------------
Should the new `OptOutPositiveFFlag` have disablers as well?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83892