Skip to content

PGO build broken on Windows #111786

Closed
Closed
PGO build broken on Windows#111786
Assignees
mdboom
Labels
OS-windowsbuildThe build process and cross-buildtype-bugAn unexpected behavior, bug, or error

Description

@mdboom
mdboom
opened on Nov 6, 2023

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:

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

Activity

  1. mdboom
    added
    type-bugAn unexpected behavior, bug, or error
    OS-windows
    buildThe build process and cross-build
    on Nov 6, 2023

  2. vstinner commented on Nov 6, 2023

    @vstinner
    vstinner
    on Nov 6, 2023
    Member

    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?

  3. mdboom
    added
    release-blocker
    on Nov 6, 2023
  4. ezio-melotti
    added this to Release and Deferred blockers 🚫on Nov 6, 2023
  5. github-project-automation
    moved this to Todo in Release and Deferred blockers 🚫on Nov 6, 2023

  6. zooba commented on Nov 6, 2023

    @zooba
    zooba
    on Nov 6, 2023
    Member

    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...

  7. mdboom
    self-assigned this
    on Nov 6, 2023

  8. mdboom commented on Nov 6, 2023

    @mdboom
    mdboom
    on Nov 6, 2023
    ContributorAuthor

    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.

  9. mdboom
    moved this from Todo to In Progress in Release and Deferred blockers 🚫on Nov 6, 2023

  10. mdboom commented on Nov 6, 2023

    @mdboom
    mdboom
    on Nov 6, 2023
    ContributorAuthor

    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.

  11. mdboom
    added a commit that references this issue on Nov 6, 2023

    pythongh-111786: Do pragmas only in MSC block

    eadfe93

  12. zooba commented on Nov 6, 2023

    @zooba
    zooba
    on Nov 6, 2023
    Member

    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.

  13. gvanrossum commented on Nov 7, 2023

    @gvanrossum
    gvanrossum
    on Nov 7, 2023
    Member

    @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.

  14. 10 remaining items

  15. neonene commented on Nov 17, 2023

    @neonene
    neonene
    on Nov 17, 2023
    Contributor

    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.

  16. mdboom commented on Nov 17, 2023

    @mdboom
    mdboom
    on Nov 17, 2023
    ContributorAuthor

    Thanks, @neonene. I will take some of these ideas, measure them, and file a PR.

  17. mdboom commented on Nov 20, 2023

    @mdboom
    mdboom
    on Nov 20, 2023
    ContributorAuthor

    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 opcodes
    • sep-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.

  18. mdboom
    added 3 commits that reference this issue on Nov 20, 2023

    pythongh-111786: Use separate opcode vars for Tier 1 and Tier 2

    00a506a

    pythongh-111786: Use separate opcode vars for Tier 1 and Tier 2

    27a5654

    pythongh-111786: Use separate opcode vars for Tier 1 and Tier 2

    f362f9a
  19. gvanrossum
    added a commit that references this issue on Nov 20, 2023

    gh-111786: Use separate opcode vars for Tier 1 and Tier 2 (#112289)

    6a00a58

  20. zooba commented on Nov 29, 2023

    @zooba
    zooba
    on Nov 29, 2023
    Member

    @mdboom Does your merged fix address this issue entirely? No point adding compatibility infrastructure if we've got a good workaround.

  21. neonene commented on Dec 4, 2023

    @neonene
    neonene
    on Dec 4, 2023
    Contributor

    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.

  22. aisk
    added 2 commits that reference this issue on Feb 11, 2024

    pythongh-111786: Optimize for space for _PyEval_EvalFrameDefault on M…

    9bfd1d8

    pythongh-111786: Use separate opcode vars for Tier 1 and Tier 2 (pyth…

    95de6df
  23. Glyphack
    added 2 commits that reference this issue on Sep 2, 2024

    pythongh-111786: Optimize for space for _PyEval_EvalFrameDefault on M…

    f1274b0

    pythongh-111786: Use separate opcode vars for Tier 1 and Tier 2 (pyth…

    c64d740
  24. mdboom
    closed this as completedon Nov 22, 2024
  25. github-project-automation
    moved this from In Progress to Done in Release and Deferred blockers 🚫on Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

OS-windowsbuildThe build process and cross-buildtype-bugAn unexpected behavior, bug, or error

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions