-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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-130213: clang-cl: blake2module.c needs to "see" the AVX intrinsics during compiling #130447
base: main
Are you sure you want to change the base?
Conversation
@@ -742,6 +744,13 @@ | |||
<Target Name="_WarnAboutToolset" BeforeTargets="PrepareForBuild" Condition="$(PlatformToolset) != 'v141' and $(PlatformToolset) != 'v142' and $(PlatformToolset) != 'v143'"> | |||
<Warning Text="Toolset $(PlatformToolset) is not used for official builds. Your build may have errors or incompatibilities." /> | |||
</Target> | |||
<Target Name="_WarnAboutTooOldClang" BeforeTargets="PrepareForBuild" Condition="'$(Platform)' == 'x64' and $(PlatformToolset) == 'ClangCL' and '$(LLVMToolsVersion)' < '19'"> |
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.
I wanted to use PrepareForClangClBuild
here like @zooba suggested, which would have saved the extra $(PlatformToolset) == 'ClangCL'
, but unfortunately this did not work.
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.
My gut feeling is that this PR is correct. It's unfortunate to do a version check, because clang has supported all of this for ages but it does look like just an oddity per the GH issue of older versions just not exposing the definitions without the compiler flag (oops!) so the version check makes sense and will work.
Converted to draft because not needed in case #130483 works out |
@@ -742,6 +744,13 @@ | |||
<Target Name="_WarnAboutToolset" BeforeTargets="PrepareForBuild" Condition="$(PlatformToolset) != 'v141' and $(PlatformToolset) != 'v142' and $(PlatformToolset) != 'v143'"> | |||
<Warning Text="Toolset $(PlatformToolset) is not used for official builds. Your build may have errors or incompatibilities." /> | |||
</Target> | |||
<Target Name="_WarnAboutTooOldClang" BeforeTargets="PrepareForBuild" Condition="'$(Platform)' == 'x64' and $(PlatformToolset) == 'ClangCL' and '$(LLVMToolsVersion)' < '19'"> |
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.
It can be a little less reliable, but doing the comparison with $([System.Version]::Parse($(LLVMToolsVersion))) < $([System.Version]::Parse('19.0'))
will at least ensure we're comparing version numbers and not garbage.
We probably also need to put the setting (and warning) in pyproject.props, to make sure that any use in any extension modules is also handled. In general, everything that applies to pythoncore.vcxproj also applies to extension modules, which is why most of the settings are in the shared file.
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.
IIUC, you mean, if we're compiling one file with /arch:AVX
, we can do for all - pythonxxx.dll will anyway have this run time dependency then?
So let's move the setting to pyproject.props - and if #129907 is merged, this will probably move to pyproject-clangcl.props?
If I am not mistaken, by moving the warning, we'd see it when building any extension module during the build process, i.e. many times. Doesn't hurt IMHO, is quite an important warning - isn't it?
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.
The reasons we need the flag could potentially apply to an extension module - e.g. if someone moves the blake module to its own file (probably where it ought to be anyway). So we ought to turn it on for everything.
Warning for everything isn't quite as important - one per build is probably fine - but it's easier to define it all in one place, and getting the warning multiple times isn't bad, so may as well keep it simple.
I'm pretty sure we'll get the other one merged soon (I haven't looked at it yet this week), so might get to move it all into pyproject-clangcl.props
before merging this one anyway.
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.
@msprotz is working on hiding the "problematic" #include "libintvector.h"
(#130483 (comment)), so most probably we won't need it just as a temporary workaround.
In the meantime, the few people using clang-cl should use versions >= 19.0.0. They'd likely do anyway, to use the tailcalling interpreter (#130040).
PR created based on @zooba's suggestion #129907 (comment) to fix compiling the
blake2module.c
on Windows with older versions of clang-cl.I think this is a skip-news because clang-cl is quite niche?