[clang] SimpleMFlag helper in Options.td

[rampitec]

Summary

This is the new helper to create a boolean -m and -mno-
options.

[kzhuravl]

If folks are ok with this approach, we probably will need to update other targets' *m* flags (e.g. -m[no-]unimplemented_simd128, -m[no-]fix_cortex_a53_835769, etc.)

[jansvoboda11]

clang/include/clang/Driver/Options.td

436

Can you please rename this to BoolMFlag and model it after BoolFFlag?

The Bool?Option multiclasses were created for fine-grained control of cc1 flag marshalling, which your implementation doesn't do/need. https://clang.llvm.org/docs/InternalsManual.html#option-marshalling-infrastructure

[rampitec]

clang/include/clang/Driver/Options.td

436

It does not seem that BoolFFlag exists?

[jansvoboda11]

clang/include/clang/Driver/Options.td

436

Ah, sorry, I meant OptInFFlag and OptOutFFlag.

You probably won't need the KeyPathAndMacro and enablers parameters, as you're not targeting cc1.

[rampitec]

clang/include/clang/Driver/Options.td

436

It end up with the same multiclass, just different name? After OptInFFlag is cleaned from marshalling, kpm, enablers and CC1 stuff it looks more or less the same as this one. Plus to get the same behavior as now we need separate help strings and optional group.

[jansvoboda11]

clang/include/clang/Driver/Options.td

436

My concern here is that naming this class BoolMOption will cause confusion. We already have BoolFOption and BoolGOption that are used for different purpose (automatic option marshalling in cc1).

Note: I'm in the process of converting Opt{In,Out}FFlag instances that perform marshalling via the kpm and enablers parameters to BoolFOption instances. My end-goal is to have clear distinction between marshalling multiclasses (ending with Option) and simple multiclasses that don't do marshalling (ending with Flag).

Since your multiclass doesn't perform marshalling, I'd strongly prefer the name to end with Flag. And since it's neither opt-in or opt-out (because it doesn't target cc1), how about naming it SimpleMFlag, PlainMFlag or something similar?

I also think keeping the API similar to existing multiclasses would be nice from user perspective. How about using the existing the Opt{In,Out}FFlag signature (stripped of kpm and enablers?

string name, string pos_prefix, string neg_prefix="", string help="", list<OptionFlag> flags=[]

It supports separate help strings and you can append OptionGroup OptGroup = m_Group at the end to fit your use-case.

I understand it will end up looking a lot like Opt{In,Out}FFlag, but the absence of CC1Option is pretty important, so I think it's fine.

[jansvoboda11]

clang/include/clang/Driver/Options.td

436

See D97370.

[kzhuravl]

clang/include/clang/Driver/Options.td

436

Would it make sense to rename OptInFFlag to OptInFlag, and take common prefix as a template arg, something like:

multiclass OptInFlag<**string prefix**, string name, string pos_prefix, string neg_prefix="",
                      string help="", list<OptionFlag> flags=[]> {

Then we will have existing "optin flags" as

defm cuda_short_ptr : OptInFlag<"f", "cuda-short-ptr",

And new "optin flags" as

defm tgsplit : OptInFlag<"m", "tgsplit",

?

[jansvoboda11]

clang/include/clang/Driver/Options.td

436

Extracting the prefix parameter is certainly possible, but I don't see how OptInFlag or OptOutFlag could be used for the options in this patch.

Opt-in means that the driver recognizes two flags (the positive and the negative), while cc1 recognizes only the positive flag. The value in CompilerInvocation controlled by the flag defaults to false and can be set to true by the positive flag (the user can "opt-in" to some behavior). Opt-out means the opposite.

If I'm not mistaken, neither of the options in this patch are recognized by cc1, so the concept of opt-in/opt-out doesn't apply here.

[rampitec]

Renamed macro as SimpleMFlag.

clang/include/clang/Driver/Options.td

436

My concern here is that naming this class BoolMOption will cause confusion. We already have BoolFOption and BoolGOption that are used for different purpose (automatic option marshalling in cc1).

Note: I'm in the process of converting Opt{In,Out}FFlag instances that perform marshalling via the kpm and enablers parameters to BoolFOption instances. My end-goal is to have clear distinction between marshalling multiclasses (ending with Option) and simple multiclasses that don't do marshalling (ending with Flag).

Since your multiclass doesn't perform marshalling, I'd strongly prefer the name to end with Flag. And since it's neither opt-in or opt-out (because it doesn't target cc1), how about naming it SimpleMFlag, PlainMFlag or something similar?

I also think keeping the API similar to existing multiclasses would be nice from user perspective. How about using the existing the Opt{In,Out}FFlag signature (stripped of kpm and enablers?

string name, string pos_prefix, string neg_prefix="", string help="", list<OptionFlag> flags=[]

It supports separate help strings and you can append OptionGroup OptGroup = m_Group at the end to fit your use-case.

I understand it will end up looking a lot like Opt{In,Out}FFlag, but the absence of CC1Option is pretty important, so I think it's fine.

I have renamed it.

With the help it is not so easy to have a single string. Look at the uses, positive and negative options have sufficiently different text.

I am also not sure about "list<OptionFlag> flags", it does not seem to have any use for the -m options.

[jansvoboda11]

Thanks for renaming the multiclass.

LGTM with the typo fixed. Decision on the final signature is up to you.

clang/include/clang/Driver/Options.td

436

The suggested signature doesn't have one help string, it has three: the pos_prefix, neg_prefix and common suffix help.
It could simplify some of your options (e.g. "Enable", "Disable" , " threadgroup split execution mode (AMDGPU only)").

I'm fine with leaving flags out as there's no need for them right now.

3160

Small typo.

[rampitec]

Fixed typo.
Made it more like OptInFFlag with 3 help strings and parameter naming.

clang/include/clang/Driver/Options.td

436

Right, I missed I can combine anything out of 3 strings. Added 3 strings like in OptInFFlag.

clang/include/clang/Driver/Options.td

 multiclass OptOutFFlag<string name, string pos_prefix, string neg_prefix,
                        string help="", list<OptionFlag> flags=[]> {
   def f#NAME : Flag<["-"], "f"#name>, Flags<flags>,
                Group<f_Group>, HelpText<!strconcat(pos_prefix, help)>;
   def fno_#NAME : Flag<["-"], "fno-"#name>, Flags<!listconcat([CC1Option], flags)>,
                   Group<f_Group>, HelpText<!strconcat(neg_prefix, help)>;
 }
 
+// Creates a positive and negative flags where both of them are prefixed with
+// "m", have help text specified for positive and negative option, and a Group
+// optionally specified by the opt_group argument, otherwise Group<m_Group>.
+multiclass SimpleMFlag<string name, string pos_prefix, string neg_prefix,
+                       string help, OptionGroup opt_group = m_Group> {
+  def m#NAME : Flag<["-"], "m"#name>, Group<opt_group>,
+    HelpText<!strconcat(pos_prefix, help)>;
+  def mno_#NAME : Flag<["-"], "mno-"#name>, Group<opt_group>,
+    HelpText<!strconcat(neg_prefix, help)>;
+}
+
 //===----------------------------------------------------------------------===//
 // BoolOption
 //===----------------------------------------------------------------------===//
 
 // The default value of a marshalled key path.
 class Default<code value> { code Value = value; }
 
 // Convenience variables for boolean defaults.
multiclass BoolGOption<string flag_base, KeyPathAndMacro kpm,
                        BothFlags both = BothFlags<[], "">> {
   defm NAME : BoolOption<"g", flag_base, kpm, default, flag1, flag2, both>,
               Group<g_Group>;
 }
 
 // FIXME: Diagnose if target does not support protected visibility.
 class MarshallingInfoVisibility<KeyPathAndMacro kpm, code default>
   : MarshallingInfoEnum<kpm, default>,
     Values<"default,hidden,internal,protected">,
     NormalizedValues<["DefaultVisibility", "HiddenVisibility",
                       "HiddenVisibility", "ProtectedVisibility"]> {}
 
 // Key paths that are constant during parsing of options with the same key path prefix.
 defvar cplusplus = LangOpts<"CPlusPlus">;
 defvar cpp11 = LangOpts<"CPlusPlus11">;
 defvar cpp17 = LangOpts<"CPlusPlus17">;
 defvar cpp20 = LangOpts<"CPlusPlus20">;

 def mexec_model_EQ : Joined<["-"], "mexec-model=">, Group<m_wasm_Features_Driver_Group>,
                      Values<"command,reactor">,
                      HelpText<"Execution model (WebAssembly only)">;
 
 def mcode_object_version_EQ : Joined<["-"], "mcode-object-version=">, Group<m_Group>,
   HelpText<"Specify code object ABI version. Defaults to 3. (AMDGPU only)">,
   MetaVarName<"<version>">, Values<"2,3,4">;
 
-def mcode_object_v3_legacy : Flag<["-"], "mcode-object-v3">, Group<m_Group>,
-  HelpText<"Legacy option to specify code object ABI V2 (-mnocode-object-v3) or V3 (-mcode-object-v3) (AMDGPU only)">;
-def mno_code_object_v3_legacy : Flag<["-"], "mno-code-object-v3">, Group<m_Group>;
-
-def mcumode : Flag<["-"], "mcumode">, Group<m_amdgpu_Features_Group>,
-  HelpText<"Specify CU (-mcumode) or WGP (-mno-cumode) wavefront execution mode (AMDGPU only)">;
-def mno_cumode : Flag<["-"], "mno-cumode">, Group<m_amdgpu_Features_Group>;
-
-def mtgsplit : Flag<["-"], "mtgsplit">, Group<m_amdgpu_Features_Group>,
-  HelpText<"Enable threadgroup split execution mode (AMDGPU only)">;
-def mno_tgsplit : Flag<["-"], "mno-tgsplit">, Group<m_amdgpu_Features_Group>,
-  HelpText<"Disable threadgroup split execution mode (AMDGPU only)">;
-
-def mwavefrontsize64 : Flag<["-"], "mwavefrontsize64">, Group<m_Group>,
-  HelpText<"Specify wavefront size 64 mode (AMDGPU only)">;
-def mno_wavefrontsize64 : Flag<["-"], "mno-wavefrontsize64">, Group<m_Group>,
-  HelpText<"Specify wavefront size 32 mode (AMDGPU only)">;
+defm code_object_v3_legacy : SimpleMFlag<"code-object-v3",
+  "Legacy option to specify code object ABI V3",
+  "Legacy option to specify code object ABI V2",
+  " (AMDGPU only)">;
+defm cumode : SimpleMFlag<"cumode",
+  "Specify CU wavefront", "Specify WGP wavefront",
+  " execution mode (AMDGPU only)", m_amdgpu_Features_Group>;
+defm tgsplit : SimpleMFlag<"tgsplit", "Enable", "Disable",
+  " threadgroup split execution mode (AMDGPU only)", m_amdgpu_Features_Group>;
+defm wavefrontsize64 : SimpleMFlag<"wavefrontsize64",
+  "Specify wavefront size 64", "Specify wavefront size 32",
+  " mode (AMDGPU only)">;
 
 defm unsafe_fp_atomics : BoolOption<"m", "unsafe-fp-atomics",
   TargetOpts<"AllowAMDGPUUnsafeFPAtomics">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable unsafe floating point atomic instructions (AMDGPU only)">,
   NegFlag<SetFalse>>, Group<m_Group>;
 
 def faltivec : Flag<["-"], "faltivec">, Group<f_Group>, Flags<[NoXarchOption]>;
 def fno_altivec : Flag<["-"], "fno-altivec">, Group<f_Group>, Flags<[NoXarchOption]>;