Investigate building the STL with only `/O2`, not `/Os`

[StephanTLavavej]

When @BillyONeal ported the STL's build system from MSBuild to CMake, he discovered that we were building the release STL with /O2 /Os and replicated that, although we marked it with a TRANSITION comment since this seemed strange and undesirable. (The STL really wants inlining.)

STL/CMakeLists.txt

Line 73 in bfa2cb2

set(VCLIBS_RELEASE_OPTIONS "/O2;/Os") # TRANSITION: Potentially remove /Os

Recently, @AlexGuteniev discovered that /Os was harming perf-critical codegen.

I attempted to remove /Os and found the following size effects on the primary DLL and static LIB (all sizes are in bytes):

Arch	File	Before, /O2 /Os	After, /O2	Change
x86	msvcp140_oss.dll	411,648	591,872	+43.8%
x86	libcpmt.lib	29,416,878	31,426,992	+6.8%
x64	msvcp140_oss.dll	553,984	648,192	+17.0%
x64	libcpmt.lib	31,704,278	33,951,088	+7.1%

+43.8% for the x86 DLL is probably more than we can get away with. +17.0% for the x64 DLL is less extreme, although 5% or 10% combined with profiling data indicating speedups in different areas would be easier to justify.

We should investigate where this size increase is coming from, and whether /Os can be removed (combined with other changes) without increasing the size so much. The fact that x86 is strongly affected leads me to guess that EH machinery is involved.

MSVC-internal notes:

The corresponding MSBuild change is surprisingly simple. In src/vctools/crt/crt_build.settings.targets:

         <ClOptimization Condition="'$(DebugBuild)' == 'true'">Disabled</ClOptimization>
         <ClOptimization Condition="'$(DebugBuild)' == 'false'">MaxSpeed</ClOptimization>
+        <ClFavorSizeOrSpeed Condition="'$(DebugBuild)' == 'false'">Speed</ClFavorSizeOrSpeed>

I believe that the /O2 /Os combination was unintentional - crt_build.settings.targets was setting ClOptimization to MaxSpeed, but there's a separate setting ClFavorSizeOrSpeed. This was being set to Size somewhere, I suspect by Microsoft.DevDiv.Native.targets.

The fix is to simply favor Speed for release mode. Changing crt_build.settings.targets affects the entire vctools/crt build which is exactly what I want to do here (although the STL cares the most about inlining, I believe that /O2 /Os is undesirable for all VCLibs). I verified that this affects the command lines being used to compile the STL (I searched for xrngdev as a uniquely-named source file):

C:\Temp\LOGS>grep -oP "((-|/)O\S+ )+(?=.*xrngdev)" plain-msbuild_x86chk.log | sort | uniq
/O2 /Os /Oy-
/O2 /Oy-
/Od /Oy-

C:\Temp\LOGS>grep -oP "((-|/)O\S+ )+(?=.*xrngdev)" updated-msbuild_x86chk.log | sort | uniq
/O2 /Ot /Oy-
/Od /Oy-

/O2 /Ot is fine as /O2 implies /Ot, see the docs. (There might be a way to omit /Ot entirely but I don't think that's worth the trouble of investigating.) ​Importantly, this verifies that /Od compilation is unaffected.

Note that both fe-components.settings.targets (for c1xx.dll) and utc.settings.targets (for c2.dll) favor Speed, so this isn't unusual, and those binaries care about performance to an extreme degree.

[BillyONeal]

Did we engage BE folks to figure out what would be necessary to tell it to inline the atomic ops without this setting? Ideally we (with __forceinline) or they (by understanding atomic ops) would fix that.

I still drool over something like _builtin_constant_p :(

[AlexGuteniev]

The fact that x86 is strongly affected leads me to guess that EH machinery is involved.

My guess it is related to /arch:IA32, which has more places for long and fast vs short and slow choice. With /arch:SSE2 the example I found in std::atomic disappears

[AlexGuteniev]

Did we engage BE folks to figure out what would be necessary to tell it to inline the atomic ops without this setting?

I think they should just fix the regression happened sometime in 19.28 : https://godbolt.org/z/6Gefs9Mfs

Variation of <atomic> is excluded by directly using repo: https://godbolt.org/z/6cf789aPv

[StephanTLavavej]

@MahmoudGSaleh thinks that it might be feasible to accept this size increase, but we'd want to check the effects for ARM64, and how other VCLibs DLLs (e.g. concrt140.dll) are affected by changing the vctools/crt configuration.

[AlexGuteniev]

I'm not sure if /O2 /Os is wrong.

Omitting /Os fixes inlining on one critical path, but we already have workaround for that, and as I mentioned, I think that case should be fixed be compiler instead.

[StephanTLavavej]

DLL size archaeology:

x64	14 Up 3	15.9	16.11	17.14	18.0
msvcp140.dll	633,152	621,600	567,336	557,728	553,584
msvcp140_1.dll	--	25,120	25,120	35,952	35,408
msvcp140_2.dll	--	199,704	186,936	280,200	278,632
msvcp140_atomic_wait.dll	--	--	57,280	50,304	48,720
msvcp140_codecvt_ids.dll	--	--	21,552	31,872	31,312
Total	633,152	846,424	858,224	956,056	947,656

x86	14 Up 3	15.9	16.11	17.14	18.0
msvcp140.dll	440,128	443,424	437,816	447,616	447,592
msvcp140_1.dll	--	22,560	22,600	33,896	33,360
msvcp140_2.dll	--	166,944	167,480	294,992	293,480
msvcp140_atomic_wait.dll	--	--	53,304	47,240	46,184
msvcp140_codecvt_ids.dll	--	--	19,896	30,824	30,288
Total	440,128	632,928	701,096	854,568	850,904

arm64	15.9	16.11	17.14	18.0
msvcp140.dll	662,512	1,365,048	1,372,216	1,380,384
msvcp140_1.dll	34,296	46,632	50,744	50,720
msvcp140_2.dll	191,560	368,184	520,248	520,232
msvcp140_atomic_wait.dll	--	115,752	86,048	85,536
msvcp140_codecvt_ids.dll	--	41,016	41,520	41,504
Total	888,368	1,936,632	2,070,776	2,078,376