Repository navigation
PGO build broken on Windows #111786
Description
Activity
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?
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...
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.
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_picklebeing 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_EvalFrameDefaulthas no downside.I still plan to report this issue to MSVC. When there is a fix in MSVC, we can add an
#ifdefto remove this workaround for that newer version of MSVC, assuming it helps performance.Reacted by Eric Snow and Steve Dower- added a commit that references this issue
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
/Ooptions 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
intin one butunsigned intin 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.That said, I'm fairly sure that PGO ignores all of those options and uses its own data, and generally it improves upon the
/Ooptions 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 --pgowith and without the pragma from Mike's PR, and the compiler error disappears when I add the pragma. So it has some effect.10 remaining items
Without this PGO workaround, 3.13's
EXTRA_CASES(version specific) seem to require theopcodevariable to be declared asuint8_t, rather than to be cast. C1001 error can be seen even before mixing tiers at 7e135a4, ifopcodeisinttype.The error went away when I tried making tier1 and tier2 have their own
opcodes (uint8_tandint). An alternative was to replaceEXTRA_CASESwithdefault:, leavingint opcodeas-is. However,#pragma optimize("t", off)was realistically needed to speed up them after all.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-opcodeuses an uint8_t for Tier 1 opcodes and a separate uint16_t for Tier 2 opcodessep-opcode_pragma_offdoes the above, but also removes the#pragma(since it's no longer needed to prevent the compiler crash)Os-globalsets/Osfor 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 neoneneReacted by neonene- The base for all of these is a recent main (2dbb2e0) which has the pragma workaround that turns off "optimize for speed" on
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 forclorlink, to recover to some degree.The hidden option worked for me in
pythoncore.vcxprojwith MSVC v.1936 and v.1938, specified at least at PGUpdate phase. Andoptimize("t",off)pragma inceval.cseems to work almost independently.MSVC v.1929 (VS2019) is fine with/without this option.
- moved this from In Progress to Done in Release and Deferred blockers 🚫
on Nov 22, 2024
Bug 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:
Error:
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