[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
This revision was automatically updated to reflect the committed changes. Closed by commit rC342977: [clang-cl] Provide separate flags for all the /O variants (authored by hans, committed by ). Changed prior to commit: https://reviews.llvm.org/D52266?vs=166862=166884#toc Repository: rC Clang https://reviews.llvm.org/D52266 Files: include/clang/Driver/CLCompatOptions.td lib/Driver/ToolChains/MSVC.cpp test/Driver/Xarch.c Index: include/clang/Driver/CLCompatOptions.td === --- include/clang/Driver/CLCompatOptions.td +++ include/clang/Driver/CLCompatOptions.td @@ -85,16 +85,19 @@ 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; -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">, - Alias; +// FIXME: Not sure /Gs really means /Gs0 (see PR39074). +def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, AliasArgs<["0"]>; +def _SLASH_Gs : CLJoined<"Gs">, + HelpText<"Set stack probe size (default 4096)">, Alias; def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">, Alias; 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; def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">, Alias; @@ -109,18 +112,43 @@ Alias; def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">, Alias; -def _SLASH_O0 : CLFlag<"O0">, Alias; -// /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; -def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">, - Alias; -def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">, - Alias; -def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias, - AliasArgs<["s"]>; -def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias, - AliasArgs<["2"]>; + +// The _SLASH_O option handles all the /O flags, but we also provide separate +// aliased options to provide separate help messages. +def _SLASH_O : CLJoined<"O">, + HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">, + MetaVarName<"">; +// FIXME: Not sure why we have -O0 here; MSVC doesn't support that. +def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">; +def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>, + HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">; +def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>, + HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">; +def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>, + HelpText<"Disable function inlining">; +def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>, + HelpText<"Only inline functions which are (explicitly or implicitly) marked inline">; +def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>, + HelpText<"Inline functions as deemed beneficial by the compiler">; +def : CLFlag<"Od">, Alias<_SLASH_O>, AliasArgs<["d"]>, + HelpText<"Disable optimization">; +def : CLFlag<"Og">, Alias<_SLASH_O>, AliasArgs<["g"]>, + HelpText<"No effect">; +def : CLFlag<"Oi">, Alias<_SLASH_O>, AliasArgs<["i"]>, + HelpText<"Enable use of builtin functions">; +def : CLFlag<"Oi-">, Alias<_SLASH_O>, AliasArgs<["i-"]>, + HelpText<"Disable use of builtin functions">; +def : CLFlag<"Os">, Alias<_SLASH_O>, AliasArgs<["s"]>, + HelpText<"Optimize for size">; +def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>, + HelpText<"Optimize for speed">; +def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>, + HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">; +def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>, + HelpText<"Enable frame pointer omission (x86 only)">; +def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>, + HelpText<"Disable frame pointer omission (x86 only, default)">; + def _SLASH_QUESTION : CLFlag<"?">, Alias, HelpText<"Display available options">; def _SLASH_Qvec : CLFlag<"Qvec">, @@ -326,10 +354,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
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans added a comment. In https://reviews.llvm.org/D52266#1244733, @thakis wrote: > > Actually, trying this out with MSVC, I don't see any __chkstk calls with > > /https://reviews.llvm.org/owners/package/1/, 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 Comment at: include/clang/Driver/CLCompatOptions.td:118 +def _SLASH_O : CLJoined<"O">, + HelpText<"Set many /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">, + MetaVarName<"">; thakis wrote: > very nit: Maybe s/many/several/ or s/many/multiple/ Ah, you noticed that. I switched to many for the 80-col limit. But multiple is better, so let's use that. https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
thakis added a comment. > Actually, trying this out with MSVC, I don't see any __chkstk calls with > /https://reviews.llvm.org/owners/package/1/, 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) https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. lgtm, thanks! Maybe file a bug on figuring out the /Gs story and add a FIXME linking to it. Weird. Comment at: include/clang/Driver/CLCompatOptions.td:118 +def _SLASH_O : CLJoined<"O">, + HelpText<"Set many /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">, + MetaVarName<"">; very nit: Maybe s/many/several/ or s/many/multiple/ https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans updated this revision to Diff 166862.
hans added a comment.
Addressing all comments.
https://reviews.llvm.org/D52266
Files:
include/clang/Driver/CLCompatOptions.td
lib/Driver/ToolChains/MSVC.cpp
test/Driver/Xarch.c
Index: test/Driver/Xarch.c
===
--- test/Driver/Xarch.c
+++ test/Driver/Xarch.c
@@ -1,10 +1,10 @@
-// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O2 %s -S -### 2>&1 | FileCheck -check-prefix=O2ONCE %s
-// O2ONCE: "-O2"
-// O2ONCE-NOT: "-O2"
+// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O3 %s -S -### 2>&1 | FileCheck -check-prefix=O3ONCE %s
+// O3ONCE: "-O3"
+// O3ONCE-NOT: "-O3"
-// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O2 %s -S -### 2>&1 | FileCheck -check-prefix=O2NONE %s
-// O2NONE-NOT: "-O2"
-// O2NONE: argument unused during compilation: '-Xarch_i386 -O2'
+// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O3 %s -S -### 2>&1 | FileCheck -check-prefix=O3NONE %s
+// O3NONE-NOT: "-O3"
+// O3NONE: argument unused during compilation: '-Xarch_i386 -O3'
// RUN: not %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -o -Xarch_i386 -S %s -S -Xarch_i386 -o 2>&1 | FileCheck -check-prefix=INVALID %s
// INVALID: error: invalid Xarch argument: '-Xarch_i386 -o'
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -1378,6 +1378,7 @@
}
break;
case 'g':
+ A->claim();
break;
case 'i':
if (I + 1 != E && OptStr[I + 1] == '-') {
Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -85,16 +85,18 @@
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;
-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">,
- Alias;
+def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, AliasArgs<["0"]>;
+def _SLASH_Gs : CLJoined<"Gs">,
+ HelpText<"Set stack probe size (default 4096)">, Alias;
def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
Alias;
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;
def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
Alias;
@@ -109,18 +111,43 @@
Alias;
def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
Alias;
-def _SLASH_O0 : CLFlag<"O0">, Alias;
-// /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;
-def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
- Alias;
-def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
- Alias;
-def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias,
- AliasArgs<["s"]>;
-def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias,
- AliasArgs<["2"]>;
+
+// The _SLASH_O option handles all the /O flags, but we also provide separate
+// aliased options to provide separate help messages.
+def _SLASH_O : CLJoined<"O">,
+ HelpText<"Set many /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">,
+ MetaVarName<"">;
+// FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
+def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
+def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
+ HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
+ HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">;
+def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
+ HelpText<"Disable function inlining">;
+def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
+ HelpText<"Only inline functions which are (explicitly or implicitly) marked inline">;
+def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>,
+ HelpText<"Inline functions as deemed beneficial by the compiler">;
+def : CLFlag<"Od">, Alias<_SLASH_O>, AliasArgs<["d"]>,
+ HelpText<"Disable optimization">;
+def : CLFlag<"Og">, Alias<_SLASH_O>,
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans marked 7 inline comments as done.
hans added a comment.
> 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
> (?)."?
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
/https://reviews.llvm.org/owners/package/1/ and
/https://reviews.llvm.org/owners/package/2/ originally.
But yes, making /https://reviews.llvm.org/owners/package/1/ and
/https://reviews.llvm.org/owners/package/2/ 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
/https://reviews.llvm.org/owners/package/1/, 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)):
: 55 pushebp
0001: 8B EC mov ebp,esp
0003: 81 EC 00 02 00 00 sub esp,200h
0009: 68 00 02 00 00 push200h
000E: 8D 85 00 FE FF FF lea eax,[ebp-200h]
0014: 6A 00 push0
0016: 50 pusheax
0017: E8 00 00 00 00 call_memset
001C: 8B 85 00 FE FF FF mov eax,dword ptr [ebp-200h]
0022: 83 C4 0C add esp,0Ch
0025: 8B E5 mov esp,ebp
0027: 5D pop ebp
0028: C3 ret
Maybe MSVC started ignoring /Gs but didn't update the docs?
https://reviews.llvm.org/D52266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
thakis added inline comments. Comment at: include/clang/Driver/CLCompatOptions.td:121 +def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>, HelpText<"Disable function inlining">; +def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>, HelpText<"Only inline functions which are (explicitly or implicitly) marked inline">; +def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>, HelpText<"Inline functions as deemed beneficial by the compiler">; 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 :-) https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
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, AliasArgs<["0"]>; +def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">, Alias; 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<"">; Move FIXME down one so it's next to the O0 flag? Comment at: include/clang/Driver/CLCompatOptions.td:118 +def : CLFlag<"O0">, Alias, 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, 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans updated this revision to Diff 166252.
hans added a comment.
Uploading new diff.
https://reviews.llvm.org/D52266
Files:
include/clang/Driver/CLCompatOptions.td
lib/Driver/ToolChains/MSVC.cpp
test/Driver/Xarch.c
Index: test/Driver/Xarch.c
===
--- test/Driver/Xarch.c
+++ test/Driver/Xarch.c
@@ -1,10 +1,10 @@
-// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O2 %s -S -### 2>&1 | FileCheck -check-prefix=O2ONCE %s
-// O2ONCE: "-O2"
-// O2ONCE-NOT: "-O2"
+// RUN: %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -O3 %s -S -### 2>&1 | FileCheck -check-prefix=O3ONCE %s
+// O3ONCE: "-O3"
+// O3ONCE-NOT: "-O3"
-// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O2 %s -S -### 2>&1 | FileCheck -check-prefix=O2NONE %s
-// O2NONE-NOT: "-O2"
-// O2NONE: argument unused during compilation: '-Xarch_i386 -O2'
+// RUN: %clang -target i386-apple-darwin9 -m64 -Xarch_i386 -O3 %s -S -### 2>&1 | FileCheck -check-prefix=O3NONE %s
+// O3NONE-NOT: "-O3"
+// O3NONE: argument unused during compilation: '-Xarch_i386 -O3'
// RUN: not %clang -target i386-apple-darwin9 -m32 -Xarch_i386 -o -Xarch_i386 -S %s -S -Xarch_i386 -o 2>&1 | FileCheck -check-prefix=INVALID %s
// INVALID: error: invalid Xarch argument: '-Xarch_i386 -o'
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -1378,6 +1378,7 @@
}
break;
case 'g':
+ A->claim();
break;
case 'i':
if (I + 1 != E && OptStr[I + 1] == '-') {
Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -85,16 +85,17 @@
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;
-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">,
- Alias;
+def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, AliasArgs<["0"]>;
+def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">, Alias;
def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
Alias;
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;
def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
Alias;
@@ -109,18 +110,26 @@
Alias;
def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
Alias;
-def _SLASH_O0 : CLFlag<"O0">, Alias;
-// /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;
-def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
- Alias;
-def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
- Alias;
-def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias,
- AliasArgs<["s"]>;
-def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias,
- AliasArgs<["2"]>;
+
+// 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<"">;
+def : CLFlag<"O0">, Alias, 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)">;
+def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>, HelpText<"Disable function inlining">;
+def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>, HelpText<"Only inline functions which are (explicitly or implicitly) marked inline">;
+def : CLFlag<"Ob2">, Alias<_SLASH_O>, AliasArgs<["b2"]>, HelpText<"Inline functions as deemed beneficial by the compiler">;
+def : CLFlag<"Od">, Alias, HelpText<"Disable optimization">;
+def : CLFlag<"Og">, Alias<_SLASH_O>, AliasArgs<["g"]>, HelpText<"No effect">;
+def : CLFlag<"Oi">,
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans added a comment. Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch. Comment at: test/Driver/Xarch.c:5 +// 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 compnerd wrote: > 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? r342636 https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
thakis added a comment. FWIW the recommendation against /Ox in my version is because of https://github.com/ulfjack/ryu/pull/70#issuecomment-412168459 https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
thakis added inline comments. Comment at: include/clang/Driver/CLCompatOptions.td:129 +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)">; + 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 /Ob0Disable function inlining /Ob1Only inline functions explicitly or implicitly marked inline /Ob2Allow 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 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 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; -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, AliasArgs<["0"]>; +def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size (default 4096)">, Alias; def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">, Alias; 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; def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">, Alias; @@ -109,18 +114,50 @@ Alias; def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">, Alias; + +// FIXME: cl.exe doesn't understand this, not sure why clang-cl does. def _SLASH_O0 : CLFlag<"O0">, Alias; -// /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; -def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">, - Alias; -def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">, - Alias; -def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias, - AliasArgs<["s"]>; -def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias, - 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
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
compnerd added inline comments. Comment at: test/Driver/Xarch.c:5 +// 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 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? https://reviews.llvm.org/D52266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52266: [clang-cl] Provide separate flags for all the /O variants
hans created this revision.
hans added reviewers: thakis, rnk.
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!
https://reviews.llvm.org/D52266
Files:
include/clang/Driver/CLCompatOptions.td
lib/Driver/ToolChains/MSVC.cpp
test/Driver/Xarch.c
Index: test/Driver/Xarch.c
===
--- test/Driver/Xarch.c
+++ test/Driver/Xarch.c
@@ -1,8 +1,8 @@
-// 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
Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -1378,6 +1378,7 @@
}
break;
case 'g':
+ A->claim();
break;
case 'i':
if (I + 1 != E && OptStr[I + 1] == '-') {
Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -109,18 +109,25 @@
Alias;
def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
Alias;
-def _SLASH_O0 : CLFlag<"O0">, Alias;
-// /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;
-def _SLASH_Oi : CLFlag<"Oi">, HelpText<"Enable use of builtin functions">,
- Alias;
-def _SLASH_Oi_ : CLFlag<"Oi-">, HelpText<"Disable use of builtin functions">,
- Alias;
-def _SLASH_Os : CLFlag<"Os">, HelpText<"Optimize for size">, Alias,
- AliasArgs<["s"]>;
-def _SLASH_Ot : CLFlag<"Ot">, HelpText<"Optimize for speed">, Alias,
- 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, 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, 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,
HelpText<"Display available options">;
def
