[clang][cli] Do not marshall only CC1Option flags in BoolOption

[jansvoboda11]

Summary

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.

[jansvoboda11]

Fix comment typo

[dexonsmith]

LGTM.

[jansvoboda11]

Add forgotten CC1Option check in parsing macro, extract denormalizer changes

[jansvoboda11]

@Bigcheese, can you please go through this again and let me know it it looks good to you now?

[Bigcheese]

lgtm.

clang/include/clang/Driver/Options.td

: FlagDef<flag.Polarity, flag.Value, flag.OptionFlags, flag.Help,
             flag.ImpliedBy> {
   // Name of the TableGen record.
   string RecordName = prefix#!cond(flag.Polarity.Value : "", true : "no_")#name;
 
   // Spelling of the flag.
   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));
 
   // C++ code that will be assigned to the keypath when the flag is present.
   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.
 // Names of the records consist of the specified prefix, "no_" for the negative
 // flag, and NAME.
 // BoolOption is the API that should be used most of the time. Use this only
 // when you need more control (e.g. to represent a marshalled option whose
 // keypath defaults to an arbitrarily complex boolean expression).
 multiclass BoolOptionBase<string spelling_base, code keypath, Default default,
                           FlagDef flag1_base, FlagDef flag2_base,
                           FlagDefSuffix flags_suffix = FlagDefSuffix<[], "">,
                           string prefix = ""> {
   defvar flag1 = FlagDefExpanded<ApplySuffix<flag1_base, flags_suffix>.Result,
                                  prefix, NAME, spelling_base>;
 
   defvar flag2 = FlagDefExpanded<ApplySuffix<flag2_base, flags_suffix>.Result,
                                  prefix, NAME, spelling_base>;
 
   // TODO: Assert that the flags have different polarity.
   // TODO: Assert that the flags have different 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>;
-  }
+  defvar implied = !cond(flag1.CanBeImplied: flag1, true: flag2);
 
-  if flag2.IsCC1 then {
-    def flag2.RecordName : CC1BoolOptionFlag<flag2, flag1, keypath, default>;
-  } else {
-    def flag2.RecordName : BoolOptionFlag<flag2, keypath, default>;
-  }
+  def flag1.RecordName : BoolOptionFlag<flag1, flag2, implied, keypath, default>;
+  def flag2.RecordName : BoolOptionFlag<flag2, flag1, implied, keypath, default>;
 }
 
 //===----------------------------------------------------------------------===//
 // BoolOption
 //===----------------------------------------------------------------------===//
 
 class DefaultsToBool<bit value> { bit Value = value; }
 def DefaultsToTrue : DefaultsToBool<true> {}
def fprofile_instrument_use_path_EQ :
     Joined<["-"], "fprofile-instrument-use-path=">,
     HelpText<"Specify the profile path in PGO use compilation">;
 def flto_visibility_public_std:
     Flag<["-"], "flto-visibility-public-std">,
     HelpText<"Use public LTO visibility for classes in std and stdext namespaces">;
 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">;
 // The driver option takes the key as a parameter to the -msign-return-address=
 // and -mbranch-protection= options, but CC1 has a separate option so we
 // don't have to parse the parameter twice.
 def msign_return_address_key_EQ : Joined<["-"], "msign-return-address-key=">,
     Values<"a_key,b_key">;

clang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
if (Opts.OptimizationLevel > 0) {
         Opts.setInlining(CodeGenOptions::NormalInlining);
       else if (InlineOpt.matches(options::OPT_finline_hint_functions))
         Opts.setInlining(CodeGenOptions::OnlyHintInlining);
       else
         Opts.setInlining(CodeGenOptions::OnlyAlwaysInlining);
     }
   }
 
-  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")
       Opts.setVecLib(CodeGenOptions::Accelerate);
     else if (Name == "libmvec")
       Opts.setVecLib(CodeGenOptions::LIBMVEC);
     else if (Name == "MASSV")
       Opts.setVecLib(CodeGenOptions::MASSV);

 
 bool CompilerInvocation::parseSimpleArgs(const ArgList &Args,
                                          DiagnosticsEngine &Diags) {
 #define OPTION_WITH_MARSHALLING(                                               \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     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);                    \
     if (auto MaybeValue = NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags))      \
       this->KEYPATH = MERGER(                                                  \
           this->KEYPATH, static_cast<decltype(this->KEYPATH)>(*MaybeValue));   \
   }
 

clang/unittests/Frontend/CompilerInvocationTest.cpp

 //===- unittests/Frontend/CompilerInvocationTest.cpp - CI tests //---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
TEST_F(CommandLineTest, BoolOptionDefaultArbitraryTwoFlagsPresentNeg) {
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
   ASSERT_THAT(GeneratedArgs,
               Not(Contains(StrEq("-fno-experimental-new-pass-manager"))));
   ASSERT_THAT(GeneratedArgs,
               Not(Contains(StrEq("-fexperimental-new-pass-manager"))));
 }
 
+// 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"};
 
   CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags);
 
   ASSERT_FALSE(Diags->hasErrorOccurred());
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);