[clang] 95114f2 - [clang][cli] Do not marshall only CC1Option flags in BoolOption
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 16 00:50:59 PST 2020
Author: Jan Svoboda
Date: 2020-12-16T09:29:40+01:00
New Revision: 95114f21f5bf1704672dadb45ca7c25efca72e03
URL: https://github.com/llvm/llvm-project/commit/95114f21f5bf1704672dadb45ca7c25efca72e03
DIFF: https://github.com/llvm/llvm-project/commit/95114f21f5bf1704672dadb45ca7c25efca72e03.diff
LOG: [clang][cli] Do not marshall only CC1Option flags in BoolOption
We cannot be sure whether a flag is CC1Option inside the definition of `BoolOption`. Take the example below:
```
let Flags = [CC1Option] in {
defm xxx : BoolOption<...>;
}
```
where TableGen applies `Flags = [CC1Option]` to the `xxx` and `no_xxx` records **after** they have been is fully declared by `BoolOption`.
For the refactored `-f[no-]debug-pass-manager` flags (see the diff), this means `BoolOption` never adds any marshalling info, as it doesn't see either of the flags as `CC1Option`.
For that reason, we should defensively append the marshalling information to both flags inside `BoolOption`. Now the check for `CC1Option` needs to happen later, in the parsing macro, when all TableGen logic has been resolved.
However, for some flags defined through `BoolOption`, we can run into issues:
```
// Options.td
def fenable_xxx : /* ... */;
// Both flags are CC1Option, the first is implied.
defm xxx : BoolOption<"xxx,
"Opts.Xxx", DefaultsToFalse,
ChangedBy<PosFlag, [CC1Option], "", [fenable_xxx]>,
ResetBy<NegFlag, [CC1Option]>>;
```
When parsing `clang -cc1 -fenable-xxx`:
* we run parsing for `PosFlag`:
* set `Opts.Xxx` to default `false`,
* discover `PosFlag` is implied by `-fenable-xxx`: set `Opts.Xxx` to `true`,
* don't see `-fxxx` on command line: do nothing,
* we run parsing for `NegFlag`:
* set `Opts.Xxx` to default `false`,
* discover `NegFlag` cannot be implied: do nothing,
* don't see `-fno-xxx` on command line: do nothing.
Now we ended up with `Opts.Xxx` set to `false` instead of `true`. For this reason, we need to ensure to append the same `ImpliedByAnyOf` instance to both flags.
This means both parsing runs now behave identically (they set the same default value, run the same "implied by" check, and call `makeBooleanOptionNormalizer` that already has information on both flags, so it returns the same value in both calls).
The solution works well, but what might be confusing is this: you have defined a flag **A** that is not `CC1Option`, but can be implied by another flag **B** that is `CC1Option`:
* if **A** is defined manually, it will never get implied, as the code never runs
```
def no_signed_zeros : Flag<["-"], "fno-signed-zeros">, Group<f_Group>, Flags<[]>,
MarshallingInfoFlag<"LangOpts->NoSignedZero">, ImpliedByAnyOf<[menable_unsafe_fp_math]>;
```
* if **A** is defined by `BoolOption`, it could get implied, as the code is run by its `CC1Option` counterpart:
```
defm signed_zeros : BoolOption<"signed-zeros",
"LangOpts->NoSignedZero", DefaultsToFalse,
ChangedBy<NegFlag, [], "Allow optimizations that ignore the sign of floating point zeros",
[cl_no_signed_zeros, menable_unsafe_fp_math]>,
ResetBy<PosFlag, [CC1Option]>, "f">, Group<f_Group>;
```
This is a surprising inconsistency.
One solution might be to somehow propagate the final `Flags` of the implied flag in `ImpliedByAnyOf` and check whether it has `CC1Option` in the parsing macro. However, I think it doesn't make sense to spend time thinking about a corner case that won't come up in real code.
An observation: it is unfortunate that the marshalling information is a part of the flag definition. If we represented it in a separate structure, we could avoid the "double parsing" problem by having a single source of truth. This would require a lot of additional work though.
Note: the original patch missed the `CC1Option` check in the parsing macro, making my reasoning here incomplete. Moreover, it contained a change to denormalization that wasn't necessarily related to these changes, so I've extracted that to a follow-up patch: D93094.
Reviewed By: dexonsmith, Bigcheese
Differential Revision: https://reviews.llvm.org/D93008
Added:
Modified:
clang/include/clang/Driver/Options.td
clang/lib/Frontend/CompilerInvocation.cpp
clang/unittests/Frontend/CompilerInvocationTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index f3aa6750b794..0da6a43b6986 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -325,10 +325,6 @@ class FlagDefExpanded<FlagDef flag, string prefix, string name, string spelling>
string Spelling
= prefix#!cond(flag.Polarity.Value : "", true : "no-")#spelling;
- // Does the flag have CC1Option?
- bit IsCC1 = !not(!empty(!filter(opt_flag, flag.OptionFlags,
- !eq(opt_flag, CC1Option))));
-
// Can the flag be implied by another flag?
bit CanBeImplied = !not(!empty(flag.ImpliedBy));
@@ -336,18 +332,14 @@ class FlagDefExpanded<FlagDef flag, string prefix, string name, string spelling>
code ValueAsCode = !cond(flag.Value : "true", true: "false");
}
-// Creates simple flag record.
-class BoolOptionFlag<FlagDefExpanded flag, code keypath, Default default>
- : Flag<["-"], flag.Spelling>, Flags<flag.OptionFlags>, HelpText<flag.Help> {}
-
-// Creates marshalled flag record.
-class CC1BoolOptionFlag<FlagDefExpanded flag, FlagDefExpanded other,
- code keypath, Default default>
+// Creates record with a marshalled flag.
+class BoolOptionFlag<FlagDefExpanded flag, FlagDefExpanded other,
+ FlagDefExpanded implied, code keypath, Default default>
: Flag<["-"], flag.Spelling>, Flags<flag.OptionFlags>, HelpText<flag.Help>,
MarshallingInfoBooleanFlag<keypath, default.Value, flag.ValueAsCode,
flag.RecordName, other.ValueAsCode,
other.RecordName, other.Spelling>,
- ImpliedByAnyOf<flag.ImpliedBy, flag.ValueAsCode> {}
+ ImpliedByAnyOf<implied.ImpliedBy, implied.ValueAsCode> {}
// Generates TableGen records for two command line flags that control the same
// keypath via the marshalling infrastructure.
@@ -370,17 +362,10 @@ multiclass BoolOptionBase<string spelling_base, code keypath, Default default,
// TODO: Assert that the flags have
diff erent value.
// TODO: Assert that only one of the flags can be implied.
- if flag1.IsCC1 then {
- def flag1.RecordName : CC1BoolOptionFlag<flag1, flag2, keypath, default>;
- } else {
- def flag1.RecordName : BoolOptionFlag<flag1, keypath, default>;
- }
-
- if flag2.IsCC1 then {
- def flag2.RecordName : CC1BoolOptionFlag<flag2, flag1, keypath, default>;
- } else {
- def flag2.RecordName : BoolOptionFlag<flag2, keypath, default>;
- }
+ defvar implied = !cond(flag1.CanBeImplied: flag1, true: flag2);
+
+ def flag1.RecordName : BoolOptionFlag<flag1, flag2, implied, keypath, default>;
+ def flag2.RecordName : BoolOptionFlag<flag2, flag1, implied, keypath, default>;
}
//===----------------------------------------------------------------------===//
@@ -4322,10 +4307,11 @@ def flto_visibility_public_std:
def flto_unit: Flag<["-"], "flto-unit">,
HelpText<"Emit IR to support LTO unit features (CFI, whole program vtable opt)">;
def fno_lto_unit: Flag<["-"], "fno-lto-unit">;
-def fdebug_pass_manager : Flag<["-"], "fdebug-pass-manager">,
- HelpText<"Prints debug information for the new pass manager">;
-def fno_debug_pass_manager : Flag<["-"], "fno-debug-pass-manager">,
- HelpText<"Disables debug printing for the new pass manager">;
+defm debug_pass_manager : BoolOption<"debug-pass-manager",
+ "CodeGenOpts.DebugPassManager", DefaultsToFalse,
+ ChangedBy<PosFlag, [], "Prints debug information for the new pass manager">,
+ ResetBy<NegFlag, [], "Disables debug printing for the new pass manager">,
+ BothFlags<[]>, "f">;
def fexperimental_debug_variable_locations : Flag<["-"],
"fexperimental-debug-variable-locations">,
HelpText<"Use experimental new value-tracking variable locations">;
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 23c4ad8dc516..1a863a739156 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -869,10 +869,6 @@ static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
}
}
- Opts.DebugPassManager =
- Args.hasFlag(OPT_fdebug_pass_manager, OPT_fno_debug_pass_manager,
- /* Default */ false);
-
if (Arg *A = Args.getLastArg(OPT_fveclib)) {
StringRef Name = A->getValue();
if (Name == "Accelerate")
@@ -3767,7 +3763,7 @@ bool CompilerInvocation::parseSimpleArgs(const ArgList &Args,
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
TABLE_INDEX) \
- { \
+ if ((FLAGS)&options::CC1Option) { \
this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE); \
if (IMPLIED_CHECK) \
this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE); \
diff --git a/clang/unittests/Frontend/CompilerInvocationTest.cpp b/clang/unittests/Frontend/CompilerInvocationTest.cpp
index 41da5affdb73..156e3393acd6 100644
--- a/clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ b/clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -238,6 +238,55 @@ TEST_F(CommandLineTest, BoolOptionDefaultArbitraryTwoFlagsPresentReset) {
ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq(PassManagerChangedByFlag))));
}
+// Boolean option that gets the CC1Option flag from a let statement (which
+// is applied **after** the record is defined):
+//
+// let Flags = [CC1Option] in {
+// defm option : BoolOption<...>;
+// }
+
+TEST_F(CommandLineTest, BoolOptionCC1ViaLetPresentNone) {
+ const char *Args[] = {""};
+
+ CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
+
+ ASSERT_FALSE(Diags->hasErrorOccurred());
+ ASSERT_FALSE(Invocation.getCodeGenOpts().DebugPassManager);
+
+ Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+ ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fdebug-pass-manager"))));
+ ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager"))));
+}
+
+TEST_F(CommandLineTest, BoolOptionCC1ViaLetPresentPos) {
+ const char *Args[] = {"-fdebug-pass-manager"};
+
+ CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
+
+ ASSERT_FALSE(Diags->hasErrorOccurred());
+ ASSERT_TRUE(Invocation.getCodeGenOpts().DebugPassManager);
+
+ Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+ ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fdebug-pass-manager")));
+ ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager"))));
+}
+
+TEST_F(CommandLineTest, BoolOptionCC1ViaLetPresentNeg) {
+ const char *Args[] = {"-fno-debug-pass-manager"};
+
+ CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
+
+ ASSERT_FALSE(Diags->hasErrorOccurred());
+ ASSERT_FALSE(Invocation.getCodeGenOpts().DebugPassManager);
+
+ Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+ ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager"))));
+ ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fdebug-pass-manager"))));
+}
+
TEST_F(CommandLineTest, CanGenerateCC1CommandLineFlag) {
const char *Args[] = {"-fmodules-strict-context-hash"};
More information about the cfe-commits
mailing list