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

[Hans Wennborg via Phabricator via cfe-commits]

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 at 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?


https://reviews.llvm.org/D52266