-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-131278: Support building using "computed gotos" for clang-cl on Windows #131279
Conversation
clang-cl on Windows. Patch by Chris Eibl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some readability comments. Do you want to actually add a What's New as well? (I don't know how wide is the audience)
Misc/NEWS.d/next/Build/2025-03-15-12-32-56.gh-issue-131278.1nd0mJ.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
I don't remember if we had a Whats New for the original clang-cl issue. We should definitely mention in the whats new that clang cl building on Windows is now possible, and say PGO too. |
Woops sorry pressed the wrong button! |
Misc/NEWS.d/next/Build/2025-03-15-12-32-56.gh-issue-131278.1nd0mJ.rst
Outdated
Show resolved
Hide resolved
@chris-eibl How do you plan to address:
Are there more PRs for those kind of optimizations on Windows? if so, we can delay everything until they are done. Otherwise, let's mention that in the What's New entry |
Building with clang-cl has been possible for a very long time: #101352. The only fixes I am aware of are just these two (and just main and current 3.14 alphas are/have been affected):
So maybe just mention PGO in the Whats New? @zooba treats clang-cl as quite optional in #129907 (comment) Shall I give it a go here and mention those three things? |
@Fidget-Spinner : your tail call for Windows #130040 (comment) came without What's new, too. Shall I add an entry, too?
I'd like to work on the many warnings clang-cl emits, but IMHO that won't go into What's new. Regarding optimizations, I think that's it atm, we can amend later, anyway. |
We don't need it, as there's already a whats new for tail call in general. |
Doc/whatsnew/3.14.rst
Outdated
@@ -1437,6 +1437,11 @@ Build changes | |||
with :c:expr:`Py_NO_LINK_LIB`. (Contributed by Jean-Christophe | |||
Fillion-Robin in :gh:`82909`.) | |||
|
|||
* building with clang-cl on Windows now supports PGO (profile guided | |||
optimization), uses ``-flto=thin`` and can be configured to use | |||
computed gotos. (Contributed by Chris Eibl in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO "computed gotos and/or tail calls" would be better here, but up to @Fidget-Spinner 's liking :)
I think we want to be careful putting too much into What's New, since we still aren't technically supporting clang-cl on Windows yet, we're really just unblocking it. No doubt if @chris-eibl sticks around it'll remain unblocked, but also, if Clang one day breaks down and MSVC is fine then we're still going to release. A simple entry (which should already be there?) stating that |
This is still true, we do not have a "clang" switch for build.bat, yet. I am fine with entirely removing the whatsnew if that makes it easier :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PCbuild changes look good, just suggesting some clarifications to the docs.
Add optimizing flag ``WITH_COMPUTED_GOTOS`` to support such builds using | ||
clang-cl on Windows. Patch by Chris Eibl. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add optimizing flag ``WITH_COMPUTED_GOTOS`` to support such builds using | |
clang-cl on Windows. Patch by Chris Eibl. | |
Add optimizing flag ``WITH_COMPUTED_GOTOS`` to Windows builds for when | |
using a compiler that supports it (currently clang-cl). Patch by Chris Eibl. |
Doc/whatsnew/3.14.rst
Outdated
@@ -1437,6 +1437,11 @@ Build changes | |||
with :c:expr:`Py_NO_LINK_LIB`. (Contributed by Jean-Christophe | |||
Fillion-Robin in :gh:`82909`.) | |||
|
|||
* building with clang-cl on Windows now supports PGO (profile guided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* building with clang-cl on Windows now supports PGO (profile guided | |
* clang-cl builds on Windows now work with ``--pgo`` (profile guided |
Doc/whatsnew/3.14.rst
Outdated
@@ -1437,6 +1437,11 @@ Build changes | |||
with :c:expr:`Py_NO_LINK_LIB`. (Contributed by Jean-Christophe | |||
Fillion-Robin in :gh:`82909`.) | |||
|
|||
* building with clang-cl on Windows now supports PGO (profile guided | |||
optimization), uses ``-flto=thin`` and can be configured to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization), uses ``-flto=thin`` and can be configured to use | |
optimization), and can be configured to use |
Is using -flto=thin
important to call out? It shouldn't change the ABI of the build output at all, and it's not configurable.
(Seen after submitting my review.) Oh don't we? Okay, yeah, let's treat all clang-cl specific changes as undocumented then. The day it gets an option is when we'd start putting notes in the |
This reverts commit 1f5b55f.
For details please see #131278.