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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
