GeezerVison is showing github.com — PGO build broken on Windows · Issue #111786 · python/cpython
PGO build broken on Windows #111786DescriptionBug report
Bug description:
Building with --pgo on Windows seems to be broken on main (853b4b5).
My hunch, since this is failing in ceval.c, is that the recent merge of the Tier 1 and Tier 2 interpreters may have made the interpreter loop function too large for MSVC to handle.
Build command:
PCbuild\build.bat --pgo -c Release
Error:
Merging C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\amd64\python313!1.pgc
C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\amd64\python313!1.pgc: Used 25.9% (15643384 / 60370944) of total space reserved. 0.0% of the counts were dropped due to overflow.
Reading PGD file 1: C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\amd64\python313.pgd
Creating library C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\amd64\python313.lib and object C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\amd64\python313.exp
Generating code
0 of 0 ( 0.0%) original invalid call sites were matched.
0 new call sites were added.
257 of 14721 ( 1.75%) profiled functions will be compiled for speed, and the rest of the functions will be compiled for size
C:\actions-runner\_work\benchmarking\benchmarking\cpython\Python\ceval.c(669): fatal error C1001: Internal compiler error. [C:\actions-runner\_work\benchmarking\benchmarking\cpython\PCbuild\pythoncore.vcxproj]
(compiler file 'D:\a\_work\1\s\src\vctools\Compiler\Utc\src\p2\main.c', line 224)
To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com/
Please choose the Technical Support command on the Visual C++
Help menu, or open the Technical Support help file for more information
link!InvokeCompilerPass()+0x10e636
link!InvokeCompilerPass()+0x10e636
link!InvokeCompilerPass()+0x10e373
link!InvokeCompilerPass()+0x10b310
link!InvokeCompilerPass()+0x10b215
link!InvokeCompilerPass()+0x102cea
Build FAILED.
Complete build log
We don't currently have a buildbot that builds with --pgo on Windows -- probably something to fix as well.
CPython versions tested on:
CPython main branch
Operating systems tested on:
Windows
Linked issue
Microsoft Developer Community MSVC bug report
Linked PRs
Reacted by Gregory P. Smith Activityvstinner commented on Nov 6, 2023
fatal error C1001: Internal compiler error.
That sounds like a MSVC bug.
To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com/
Can you try to report the bug to MSVC support? zooba commented on Nov 6, 2023 Yeah, we'll need a link repro for this, but it should be easy enough to get them to diagnose it. Reporting through dev community is still the best option, even if you work there 😉
Hopefully there's a resolution that doesn't require a specific compiler version. Our infrastructure needs a revamp to handle that situation... mdboom commented on Nov 6, 2023 ContributorAuthor More actions My plan is to explore configuration/pragma options to see if anything can be worked around (and benchmark the effect of that so we know what the performance impact of not doing certain optimizations on certain functions is). Barring that we may need to revert or redesign #111428.
Reporting the bug to MSVC also makes sense, and I plan to do that. But I don't think we can count on it to solve our issue -- there will always be a need to compile on a range of versions for a variety of reasons, including what @zooba already said. mdboom commented on Nov 6, 2023 ContributorAuthor More actions Reading the description of error C1001, it's clear this is an error in the optimizer. This lead me to a description of MSVC's optimization pragmas as a way to control the optimization of just this one very large function (rather than spending time on anything more global).
The two main optimization flags are size ("s") and time ("t"). If you disable them both, you create a new problem, which is that the stack frames get larger, and it exceeds the C stack space during some unit tests (test_pickle being one of them). If you disable only size, the internal compiler error remains. However, if you just disable time ("t"), it successfully compiles.
It would seems that disabling "optimize for speed" would make things slower, however, running on our benchmarking machine, it is about 3% faster (by the HPT method, which is more stable between runs) or 6% faster by the geometric mean method.
So it seems like disabling the "t" optimization for just _PyEval_EvalFrameDefault has no downside.
I still plan to report this issue to MSVC. When there is a fix in MSVC, we can add an #ifdef to remove this workaround for that newer version of MSVC, assuming it helps performance. Reacted by Eric Snow and Steve Dower zooba commented on Nov 6, 2023
It would seems that disabling "optimize for speed" would make things slower, however, running on our benchmarking machine, it is about 3% faster
Large functions are often faster when optimised for size, as the instruction cache pressure tends to outweigh anything else.
That said, I'm fairly sure that PGO ignores all of those options and uses its own data, and generally it improves upon the /O options by 10-20%, if not more. So it may not be a suitable choice.
I've also found in the past that many of our optimiser crashes have been due to mishandled types between object file (e.g. a function argument is defined as int in one but unsigned int in the other). Might be worth looking for anywhere new that kind of thing may have happened - the compiler should never crash, but it's potentially crashing because of an error in our code that we'd want to fix anyway. gvanrossum commented on Nov 7, 2023 @zooba
That said, I'm fairly sure that PGO ignores all of those options and uses its own data, and generally it improves upon the /O options by 10-20%, if not more. So it may not be a suitable choice.
How do you reconcile that with @mdboom's actual benchmark results? IIUC the benchmark suite is run with PGO. And I just tried build.bat --pgo with and without the pragma from Mike's PR, and the compiler error disappears when I add the pragma. So it has some effect. neonene commented on Nov 17, 2023 Without this PGO workaround, 3.13's EXTRA_CASES (version specific) seem to require the opcode variable to be declared as uint8_t, rather than to be cast. C1001 error can be seen even before mixing tiers at 7e135a4, if opcode is int type.
The error went away when I tried making tier1 and tier2 have their own opcodes (uint8_t and int). An alternative was to replace EXTRA_CASES with default:, leaving int opcode as-is. However, #pragma optimize("t", off) was realistically needed to speed up them after all. mdboom commented on Nov 17, 2023 ContributorAuthor More actions Thanks, @neonene. I will take some of these ideas, measure them, and file a PR. mdboom commented on Nov 20, 2023 ContributorAuthor More actions I measured this a few different ways and compared
- The base for all of these is a recent main (2dbb2e0) which has the pragma workaround that turns off "optimize for speed" on
_PyEval_EvalFrameDefault
sep-opcode uses an uint8_t for Tier 1 opcodes and a separate uint16_t for Tier 2 opcodessep-opcode_pragma_off does the above, but also removes the #pragma (since it's no longer needed to prevent the compiler crash)Os-global sets /Os for all code during the PGInstrument phase as @neonene suggested as a workaround
| commit |
change |
| sep-opcode (48b46e7) |
1.03x faster (100% rel.) |
| sep-opcode_pragma-off (c47be17) |
1.00x faster (85% rel.) |
| Os-global (04300ec) |
1.00x slower (89% rel.) |
So it seems like the best thing for performance is to separate the opcode variables, leaving everything else the same. Reacted by Guido van Rossum and neonene Reacted by neonene zooba commented on Nov 29, 2023 @mdboom Does your merged fix address this issue entirely? No point adding compatibility infrastructure if we've got a good workaround. neonene commented on Dec 4, 2023 It appears that Windows PGO builds in faster-cpython/benchmarking-public repo have slowed down with Tier 2 off, after generated cases were sorted (1619f43).
If the switch-cases cannot be arranged for working around, a similar performance issue closed with low priority in MSVC's bug tracker would be worth checking, where they suggested ssa-pre- option for cl or link, to recover to some degree.
The hidden option worked for me in pythoncore.vcxproj with MSVC v.1936 and v.1938, specified at least at PGUpdate phase. And optimize("t",off) pragma in ceval.c seems to work almost independently.
MSVC v.1929 (VS2019) is fine with/without this option.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|