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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In https://reviews.llvm.org/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 /https://reviews.llvm.org/owners/package/1/ and /https://reviews.llvm.org/owners/package/2/ imply /Gs -- we keep it at the default value of 4096 (?) Fixing this probably means increasing chrome's size (?)."?



================
Comment at: include/clang/Driver/CLCompatOptions.td:94
+def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias<mstack_probe_size>, AliasArgs<["0"]>;
+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">,
----------------
Want to add "(default 4096)" here?


================
Comment at: include/clang/Driver/CLCompatOptions.td:115
+// The _SLASH_O option handles all the /O flags, but we also provide separate aliased options to provide separate help messages.
+// FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
+def _SLASH_O : CLJoined<"O">, HelpText<"Set several /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">, MetaVarName<"<flags>">;
----------------
Move FIXME down one so it's next to the O0 flag?


================
Comment at: include/clang/Driver/CLCompatOptions.td:118
+def : CLFlag<"O0">, Alias<O0>, HelpText<"Disable optimization">;
+def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>, HelpText<"Optimize for small size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>, HelpText<"Optimize for maximum speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">;
----------------
Nit: Just "optimize for size" / "optimize for speed" is shorter


================
Comment at: include/clang/Driver/CLCompatOptions.td:123
+def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>, HelpText<"Inline functions as deemed beneficial by the compiler">;
+def : CLFlag<"Od">, Alias<O0>, HelpText<"Disable optimization">;
+def : CLFlag<"Og">, Alias<_SLASH_O>, AliasArgs<["g"]>, HelpText<"No effect">;
----------------
Why not alias this to _SLASH_O, d like the rest?


================
Comment at: include/clang/Driver/CLCompatOptions.td:128
+def : CLFlag<"Os">, Alias<_SLASH_O>, AliasArgs<["s"]>, HelpText<"Optimize for small size over speed">;
+def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>, HelpText<"Optimize for speed over small size">;
+def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>, HelpText<"Deprecated; use /O2 instead (equivalent to /Og /Oi /Ot /Oy /Ob2)">;
----------------
nit: I liked the old help text for the previous 4 more, it was more concise


================
Comment at: include/clang/Driver/CLCompatOptions.td:129
+def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>, HelpText<"Optimize for speed over small size">;
+def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>, HelpText<"Deprecated; use /O2 instead (equivalent to /Og /Oi /Ot /Oy /Ob2)">;
+def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>, HelpText<"Enable frame pointer omission (x86 only)">;
----------------
nit: With this punctuation it looks ambiguous to me if the parenthetical refers to /O2 or /Ox.


https://reviews.llvm.org/D52266