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

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 added a comment.

I wanted to defer the resolution of the todos after all existing patches (https://reviews.llvm.org/search/query/wPZcRaH7zHgu/#R) are upstreamed to avoid conflicts when rebasing and refactoring. Looking at the patches more closely, only two of them seem to be dealing with boolean flags, so we might as well come up with the right naming convention and put it in a prep patch. Below are my notes and though process.

The naming convention used upstream works like this:

  +-----------------------------------+-------------+
  |        Keypath value with:        |             |
  +-----------+-----------+-----------+  Multiclass |
  |     ''    |  '-foo'   | '-no-foo' |             |
  +-----------+-----------+-----------+-------------+
  |   false   |   false   |   true    | OptOutFFlag |
  |   false   |   true    |   false   | OptInFFlag  |
  |   true    |   false   |   true    |             |
  |   true    |   true    |   false   |             |
  +-----------+-----------+-----------+-------------+

To decide what multiclass to use, you ask:

- Both `Opt{Out,In}FFlag` multiclasses default the keypath to false. What command-line flag makes it possible to change the default value of the keypath?
  - `OptIn`  = `-ffoo`
  - `OptOut` = `-fno-foo`

This patch currently works like so:

  +-----------------------------------+---------------------+
  |        Keypath value with:        |                     |
  +-----------+-----------+-----------+      Multiclass     |
  |     ''    |  '-foo'   | '-no-foo' |                     |
  +-----------+-----------+-----------+---------------------+
  |   false   |   false   |   true    | OptInNegativeFFlag  |
  |   false   |   true    |   false   | OptInPositiveFFlag  |
  |   true    |   false   |   true    | OptOutNegativeFFlag |
  |   true    |   true    |   false   | OptOutPositiveFFlag |
  +-----------+-----------+-----------+---------------------+

To decide what multiclass to use, you'd ask two questions:

- What is the semantics of the keypath?
  - `Positive` = positive, e.g. `DoThis`
  - `Negative` = negative, e.g. `NoDoThis`

- What is the default value?
  - `OptIn`  = keypath defaults to false; it can be set to true  with `-ffoo`    if it has positive semantics, or with `-fno-foo` if it has negative semantics
  - `OptOut` = keypath defaults to true; it can be set to false with `-fno-foo` if it has positive semantics, or with `-ffoo`    if it has negative semantics

I agree the axes and their naming is confusing. So how to make the behavior and usage clearer?

I think it's necessary to drop the assumption that exactly one of the flags is available in `-cc1`. Some of the record pairs I've marked with a todo don't conform to this scheme (e.g. `f[no_]sanitize_address_use_after_scope`) but it would still be nice to tie them together via a multiclass. I think it would now make sense to drop the `Opt{In,Out}` wording.

I think the simplest way to think about this is to encode these properties:

- What is the keypath default? Is it `true` or `false`?
- Which flag is used to invert the default keypath value? Is it `-ffoo` or `-fno-foo`?

How about something like this?

  +-----------------------------------+-----------------------------------------------------+
  |        Keypath value with:        |                                                     |
  +-----------+-----------+-----------+                      Multiclass                     |
  |     ''    |  '-foo'   | '-no-foo' |                                                     |
  +-----------+-----------+-----------+-----------------------------------------------------+
  |   false   |   false   |   true    | BoolOption<DefaultsToFalse, InvertedByNegativeFlag> |
  |   false   |   true    |   false   | BoolOption<DefaultsToFalse, InvertedByPositiveFlag> |
  |   true    |   false   |   true    | BoolOption<DefaultsToTrue, InvertedByPositiveFlag>  |
  |   true    |   true    |   false   | BoolOption<DefaultsToTrue, InvertedByNegativeFlag>  |
  +-----------+-----------+-----------+-----------------------------------------------------+

I think it's really similar to the approach you outlined, but takes more keypath-centric approach.
To decide whether we can get away with one parametrized multiclass, I'd need to dig a bit deeper into TableGen.

Another problem to solve: how to declare a mixin that only applies to one of the two generated records?

Option 1:

Linear list of arguments with default values:

  multiclass BoolOption<??? defaults_to, ??? inverted_by, string keypath, list<OptionFlag> pos_flags = [], string pos_help = "", list<OptionFlag> neg_flags = [], string neg_help ="", list<OptionFlag> both_flags = [], string both_help_suffix = ""> { ... }

The downside here is that this gets hard to understand:

  defm inline_line_tables : BoolGOption<DefaultsToFalse, InvertedByNegativeFlag,
    "CodeGenOpts.NoInlineLineTables", [], "", [CC1Option], "Don't emit inline line tables", [CoreOption], "">;

This could be solved by named arguments, but I'm not sure TableGen supports them.

Option 2:

Group the `pos_*`, `neg_*`, `both_*` arguments into `PositiveFlag`, `NegativeFlag`, `BothFlags` bags:

  defm inline_line_tables : BoolGOption<DefaultsToFalse, InvertedByNegativeFlag,
    "CodeGenOpts.NoInlineLineTables", PositiveFlag<>,
    NegativeFlag<[CC1Option], "Don't emit inline line tables">, BothFlags<[CoreOption]>>;

@dexonsmith What do you think about `DefaultsTo{True,False}` and `InvertedBy{Negative,Positive}Flag`?



================
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="",
----------------
dexonsmith wrote:
> 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).
Fair enough.


================
Comment at: clang/include/clang/Driver/Options.td:293
                MarshallingInfoFlag<keypath, "false">,
                ImpliedByAnyOf<disablers, "true">;
 }
----------------
dexonsmith wrote:
> Should the new `OptOutPositiveFFlag` have disablers as well?
Probably yes, it's mentioned in a todo above. I wanted to add this only when an option needs it, to minimize untested code paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83892