[clang-cl] Provide separate flags for all the /O variants

[hans]

Summary

This provides better help text in "clang-cl /?".

Also it cleans things up a bit: previously "/Od" could be handled either as a separate flag aliased to "-O0", or by the main optimization flag processing in TranslateOptArg. With this patch, all the flags get aliased back to /O so they're handled by TranslateOptArg.

Thanks to Nico for the idea!

[compnerd]

test/Driver/Xarch.c

5

I know that this isn’t your doing, but while you’re here, would you mind replacing the temp files with pipes and grep with FileCheck?

[thakis]

include/clang/Driver/CLCompatOptions.td

129

I took a stab at this myself for a bit, and did a few things differently (but most the same way).

Here's how they /? output looks in my local binary:

/O1                     Minimize size, equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy
/O2                     Maximize speed, equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy
/Ob0                    Disable function inlining
/Ob1                    Only inline functions explicitly or implicitly marked inline
/Ob2                    Allow compiler to inline all functions it deems beneficial
/Od                     Disable optimization
/Og                     No effect
/Oi-                    Disable use of builtin functions
/Oi                     Enable use of builtin functions
/Os                     Optimize for size
/Ot                     Optimize for speed
/Ox                     Deprecated, use O2 instead; equivalent to /Og /Oi /Ot /Oy /Ob2
/Oy-                    Don't omit frame pointers (default)
/Oy                     Omit frame pointers (x86 only)
/O<value>               Set several /O flags at once; '/O2y-' is the same as '/O2 /y-' for example

Since we reference them now, I also added these:

  /GF                     Enable string pooling (default)
...
  /Gs                     Same as /Gs0
  /Gs<value>              Set stack probe size (default 4096)
...
  /Gy-                    Don't put each function in its own section (default)
  /Gy                     Put each function in its own section

As far as I can tell we do not make /O1 and /O2 imply /Gs -- we keep it at the default value of 4096 (?) Fixing this probably means increasing chrome's size (?).

cl.exe doesn't support O0; I wonder why rnk added it. I asked on the revision that added it.

The name after def isn't needed; maybe we should omit the names of flags we don't reference?

Here's my local diff, mix and match as you see fit:

Index: include/clang/Driver/CLCompatOptions.td
===================================================================
--- include/clang/Driver/CLCompatOptions.td	(revision 342334)
+++ include/clang/Driver/CLCompatOptions.td	(working copy)
@@ -85,16 +85,21 @@
   HelpText<"Assume thread-local variables are defined in the executable">;
 def _SLASH_GR : CLFlag<"GR">, HelpText<"Enable emission of RTTI data">;
 def _SLASH_GR_ : CLFlag<"GR-">, HelpText<"Disable emission of RTTI data">;
+def _SLASH_GF : CLIgnoredFlag<"GF">,
+  HelpText<"Enable string pooling (default)">;
 def _SLASH_GF_ : CLFlag<"GF-">, HelpText<"Disable string pooling">,
   Alias<fwritable_strings>;
-def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check">;
+def _SLASH_GS : CLFlag<"GS">,
+  HelpText<"Enable buffer security check (default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">,
+def: CLFlag<"Gs">, HelpText<"Same as /Gs0">,
+  Alias<mstack_probe_size>, AliasArgs<["0"]>;
+def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size (default 4096)">,
   Alias<mstack_probe_size>;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
   Alias<ffunction_sections>;
 def _SLASH_Gy_ : CLFlag<"Gy-">,
-  HelpText<"Don't put each function in its own section">,
+  HelpText<"Don't put each function in its own section (default)">,
   Alias<fno_function_sections>;
 def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
   Alias<fdata_sections>;
@@ -109,18 +114,50 @@
   Alias<I>;
 def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
   Alias<funsigned_char>;
+
+// FIXME: cl.exe doesn't understand this, not sure why clang-cl does.
 def _SLASH_O0 : CLFlag<"O0">, Alias<O0>;
-// /Oy- is handled by the /O option because /Oy- only has an effect on 32-bit.
-def _SLASH_O : CLJoined<"O">, HelpText<"Optimization level">;
-def _SLASH_Od : CLFlag<"Od">, HelpText<"Disable optimization">, Alias<O0>;
-def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
-  Alias<fbuiltin>;
-def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
-  Alias<fno_builtin>;
-def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias<O>,
-  AliasArgs<["s"]>;
-def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias<O>,
-  AliasArgs<["2"]>;
+
+def _SLASH_O : CLJoined<"O">,
+  HelpText<"Set several /O flags at once; "
+           "'/O2y-' is the same as '/O2 /y-' for example">;
+
+// FIXME: Looks like this doesn't actually imply /Gs in clang-cl yet.
+def _SLASH_O1 : CLFlag<"O1">,
+  HelpText<"Minimize size, equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy">,
+  Alias<_SLASH_O>, AliasArgs<["1"]>;
+def _SLASH_O2 : CLFlag<"O2">,
+  HelpText<"Maximize speed, equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy">,
+  Alias<_SLASH_O>, AliasArgs<["2"]>;
+def _SLASH_Ox : CLFlag<"Ox">,
+  HelpText<"Deprecated, use O2 instead; equivalent to /Og /Oi /Ot /Oy /Ob2">,
+  Alias<_SLASH_O>, AliasArgs<["x"]>;
+def _SLASH_Od : CLFlag<"Od">, HelpText<"Disable optimization">,
+  Alias<_SLASH_O>, AliasArgs<["d"]>;
+
+def: CLFlag<"Ob0">, HelpText<"Disable function inlining">,
+  Alias<_SLASH_O>, AliasArgs<["b0"]>;
+def: CLFlag<"Ob1">,
+  HelpText<"Only inline functions explicitly or implicitly marked inline">,
+  Alias<_SLASH_O>, AliasArgs<["b1"]>;
+def: CLFlag<"Ob2">,
+  HelpText<"Allow compiler to inline all functions it deems beneficial">,
+  Alias<_SLASH_O>, AliasArgs<["b2"]>;
+def: CLFlag<"Og">, HelpText<"No effect">,
+  Alias<_SLASH_O>, AliasArgs<["g"]>;
+def: CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
+  Alias<_SLASH_O>, AliasArgs<["i"]>;
+def: CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
+  Alias<_SLASH_O>, AliasArgs<["i-"]>;
+def: CLFlag<"Os">, HelpText<"Optimize for size">,
+  Alias<_SLASH_O>, AliasArgs<["s"]>;
+def: CLFlag<"Ot">, HelpText<"Optimize for speed">,
+  Alias<_SLASH_O>, AliasArgs<["t"]>;
+def: CLFlag<"Oy">, HelpText<"Omit frame pointers (x86 only)">,
+  Alias<_SLASH_O>, AliasArgs<["y"]>;
+def: CLFlag<"Oy-">, HelpText<"Don't omit frame pointers (default)">,
+  Alias<_SLASH_O>, AliasArgs<["y-"]>;
+
 def _SLASH_QUESTION : CLFlag<"?">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_Qvec : CLFlag<"Qvec">,
@@ -326,10 +363,8 @@
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
-def _SLASH_GF : CLIgnoredFlag<"GF">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
-def _SLASH_Og : CLIgnoredFlag<"Og">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
 def _SLASH_permissive_ : CLIgnoredFlag<"permissive-">;
 def _SLASH_RTC : CLIgnoredJoined<"RTC">;

[thakis]

FWIW the recommendation against /Ox in my version is because of https://github.com/ulfjack/ryu/pull/70#issuecomment-412168459

[hans]

Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch.

test/Driver/Xarch.c

5

r342636

[hans]

Uploading new diff.

[thakis]

In D52266#1240304, @hans wrote:

Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch.

No worries, I didn't do anything I wouldn't have done for reviewing this :-)

Thoughts on "As far as I can tell we do not make /O1 and /O2 imply /Gs -- we keep it at the default value of 4096 (?) Fixing this probably means increasing chrome's size (?)."?

include/clang/Driver/CLCompatOptions.td

94

Want to add "(default 4096)" here?

114

Move FIXME down one so it's next to the O0 flag?

117

Nit: Just "optimize for size" / "optimize for speed" is shorter

122

Why not alias this to _SLASH_O, d like the rest?

127

nit: I liked the old help text for the previous 4 more, it was more concise

128

nit: With this punctuation it looks ambiguous to me if the parenthetical refers to /O2 or /Ox.

[thakis]

include/clang/Driver/CLCompatOptions.td

120

Also, can we try to keep these at 80 columns?

The other flags in this file put HelpText before Alias and AliasArg (see my diff), but your order is fine. The 80 columns help trying to keep the help text concise, so it encourages better UI in this case :-)

[hans]

Thoughts on "As far as I can tell we do not make /O1 and /O2 imply /Gs -- we keep it at the default value of 4096 (?) Fixing this probably means increasing chrome's size (?)."?

The -mstack-probe-size option and /Gs was added later than the /O options (in r226601) so that's why it wasn't hooked up to /O1 and /O2 originally.

But yes, making /O1 and /O2 imply /Gs seems like a bad idea. That would mean emitting __chkstk in every function, increasing both size and slowing down execution. Even MSVC's docs say "/Gs0 activates stack probes for every function call that requires storage for local variables. This can have a negative impact on performance." (and "If the /Gs option is specified without a size argument, it is the same as specifying /Gs0").

Actually, trying this out with MSVC, I don't see any __chkstk calls with /O1, or with eg. /Gs1 for that matter:

a.cc:
int f(int x) { volatile int a[128] = {0}; return a[0]; 

cl /c /O1 a.cc && dumpbin /disasm a.obj

...

?f@@YAHH@Z (int __cdecl f(int)):
  00000000: 55                 push        ebp
  00000001: 8B EC              mov         ebp,esp
  00000003: 81 EC 00 02 00 00  sub         esp,200h
  00000009: 68 00 02 00 00     push        200h
  0000000E: 8D 85 00 FE FF FF  lea         eax,[ebp-200h]
  00000014: 6A 00              push        0
  00000016: 50                 push        eax
  00000017: E8 00 00 00 00     call        _memset
  0000001C: 8B 85 00 FE FF FF  mov         eax,dword ptr [ebp-200h]
  00000022: 83 C4 0C           add         esp,0Ch
  00000025: 8B E5              mov         esp,ebp
  00000027: 5D                 pop         ebp
  00000028: C3                 ret

Maybe MSVC started ignoring /Gs but didn't update the docs?

[hans]

Addressing all comments.

[thakis]

lgtm, thanks!

Maybe file a bug on figuring out the /Gs story and add a FIXME linking to it. Weird.

include/clang/Driver/CLCompatOptions.td

116

very nit: Maybe s/many/several/ or s/many/multiple/

[thakis]

Actually, trying this out with MSVC, I don't see any __chkstk calls with /O1, or with eg. /Gs1 for that matter:

(.guess the bug should also cover to eventually not alias /Gs to /Gs0 and/or /Gs to -mstack-probe-size either if cl doesn't do that)

[hans]

In D52266#1244733, @thakis wrote:

Actually, trying this out with MSVC, I don't see any __chkstk calls with /O1, or with eg. /Gs1 for that matter:

(.guess the bug should also cover to eventually not alias /Gs to /Gs0 and/or /Gs to -mstack-probe-size either if cl doesn't do that)

Filed https://bugs.llvm.org/show_bug.cgi?id=39074

include/clang/Driver/CLCompatOptions.td

116

Ah, you noticed that. I switched to many for the 80-col limit. But multiple is better, so let's use that.

include/clang/Driver/CLCompatOptions.td

 def _SLASH_GR : CLFlag<"GR">, HelpText<"Enable emission of RTTI data">;
 def _SLASH_GR_ : CLFlag<"GR-">, HelpText<"Disable emission of RTTI data">;
 def _SLASH_GF_ : CLFlag<"GF-">, HelpText<"Disable string pooling">,
   Alias<fwritable_strings>;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
 def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">,
   Alias<mstack_probe_size>;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
   Alias<ffunction_sections>;
 def _SLASH_Gy_ : CLFlag<"Gy-">,
   HelpText<"Don't put each function in its own section">,
   Alias<fno_function_sections>;
 def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
   Alias<fdata_sections>;
 def _SLASH_Gw_ : CLFlag<"Gw-">,
   HelpText<"Don't put each data item in its own section">,
   Alias<fno_data_sections>;
 def _SLASH_help : CLFlag<"help">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_HELP : CLFlag<"HELP">, Alias<help>;
 def _SLASH_I : CLJoinedOrSeparate<"I">,
   HelpText<"Add directory to include search path">, MetaVarName<"<dir>">,
   Alias<I>;
 def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
   Alias<funsigned_char>;
-def _SLASH_O0 : CLFlag<"O0">, Alias<O0>;
-// /Oy- is handled by the /O option because /Oy- only has an effect on 32-bit.
-def _SLASH_O : CLJoined<"O">, HelpText<"Optimization level">;
-def _SLASH_Od : CLFlag<"Od">, HelpText<"Disable optimization">, Alias<O0>;
-def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
-  Alias<fbuiltin>;
-def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
-  Alias<fno_builtin>;
-def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias<O>,
-  AliasArgs<["s"]>;
-def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias<O>,
-  AliasArgs<["2"]>;
+
+// The _SLASH_O option handles all the /O variants, but we also provide separate aliased options to provide separate help messages.
+def _SLASH_O : CLJoined<"O">, HelpText<"Set the optimization level">, MetaVarName<"[0|1|2|b{0123}|d|g|i[-]|s|t|x|y[-]]">;
+def _SLASH_O0 : CLFlag<"O0">, Alias<O0>, HelpText<"Disable optimization">;
+def _SLASH_O1 : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>, HelpText<"Optimize for small size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+def _SLASH_O2 : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>, HelpText<"Optimize for maximum speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">;
+def _SLASH_Ob0 : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>, HelpText<"Disable inlining">;
+def _SLASH_Ob1 : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>, HelpText<"Inline functions which are (explicitly or implicitly) marked inline">;
+def _SLASH_Ob2 : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>, HelpText<"Inline suitable functions">;
+def _SLASH_Od : CLFlag<"Od">, Alias<O0>, HelpText<"Disable optimization">;
+def _SLASH_Og : CLFlag<"Og">, Alias<_SLASH_O>, AliasArgs<["g"]>;
+def _SLASH_Oi : CLFlag<"Oi">, Alias<_SLASH_O>, AliasArgs<["i"]>, HelpText<"Optimize using builtin knowledge of functions">;
+def _SLASH_Oi_ : CLFlag<"Oi-">, Alias<_SLASH_O>, AliasArgs<["i-"]>, HelpText<"Disable optimization based on builtin knowledge of functions">;
+def _SLASH_Os : CLFlag<"Os">, Alias<_SLASH_O>, AliasArgs<["s"]>, HelpText<"Optimize for small size over speed">;
+def _SLASH_Ot : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>, HelpText<"Optimize for speed over small size">;
+def _SLASH_Ox : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>, HelpText<"Optimize maximum speed (equivalent to /Og /Oi /Ot /Oy /Ob2)">;
+def _SLASH_Oy : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>, HelpText<"Enable frame pointer omission (x86 only)">;
+def _SLASH_Oy_ : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>, HelpText<"Disable frame pointer omission (x86 only)">;
+
 def _SLASH_QUESTION : CLFlag<"?">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_Qvec : CLFlag<"Qvec">,
   HelpText<"Enable the loop vectorization passes">, Alias<fvectorize>;
 def _SLASH_Qvec_ : CLFlag<"Qvec-">,
   HelpText<"Disable the loop vectorization passes">, Alias<fno_vectorize>;
 def _SLASH_showIncludes : CLFlag<"showIncludes">,
   HelpText<"Print info about included files to stderr">,

 def _SLASH_d2Zi_PLUS : CLIgnoredFlag<"d2Zi+">;
 def _SLASH_errorReport : CLIgnoredJoined<"errorReport">;
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
 def _SLASH_GF : CLIgnoredFlag<"GF">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
-def _SLASH_Og : CLIgnoredFlag<"Og">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
 def _SLASH_permissive_ : CLIgnoredFlag<"permissive-">;
 def _SLASH_RTC : CLIgnoredJoined<"RTC">;
 def _SLASH_sdl : CLIgnoredFlag<"sdl">;
 def _SLASH_sdl_ : CLIgnoredFlag<"sdl-">;
 def _SLASH_utf8 : CLIgnoredFlag<"utf-8">,
   HelpText<"Set source and runtime encoding to UTF-8 (default)">;
 def _SLASH_w : CLIgnoredJoined<"w">;

lib/Driver/ToolChains/MSVC.cpp

case 'b':
         case '2':
           DAL.AddFlagArg(A, Opts.getOption(options::OPT_finline_functions));
           break;
         }
         ++I;
       }
       break;
     case 'g':
+      A->claim();
       break;
     case 'i':
       if (I + 1 != E && OptStr[I + 1] == '-') {
         ++I;
         DAL.AddFlagArg(A, Opts.getOption(options::OPT_fno_builtin));
       } else {
         DAL.AddFlagArg(A, Opts.getOption(options::OPT_fbuiltin));
       }

test/Driver/Xarch.c

-// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O2 %s -S -### 2> %t.log
-// RUN: grep ' "-O2" ' %t.log | count 1
-// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O2 %s -S -### 2> %t.log
-// RUN: not grep ' "-O2" ' %t.log
-// RUN: grep "argument unused during compilation: '-Xarch_i386 -O2'" %t.log
+// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O3 %s -S -### 2> %t.log
+// RUN: grep ' "-O3" ' %t.log | count 1
+// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O3 %s -S -### 2> %t.log
+// RUN: not grep ' "-O3" ' %t.log
+// RUN: grep "argument unused during compilation: '-Xarch_i386 -O3'" %t.log
 // RUN: not %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -o -Xarch_i386 -S %s -S -Xarch_i386 -o 2> %t.log
 // RUN: grep "error: invalid Xarch argument: '-Xarch_i386 -o'" %t.log | count 2
 // RUN: grep "error: invalid Xarch argument: '-Xarch_i386 -S'" %t.log