Skip to content

GH-130213: clang-cl: blake2module.c needs to "see" the AVX intrinsics during compiling#130447

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.

Sign up for GitHub

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

Closed
chris-eibl wants to merge 1 commit into from
Closed

GH-130213: clang-cl: blake2module.c needs to "see" the AVX intrinsics during compiling #130447

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions PCbuild/pythoncore.vcxproj
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -420,6 +420,8 @@
<ClCompile Include="..\Modules\blake2module.c">
<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD128;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD256;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<!-- GH-130213: blake2module.c needs to "see" the AVX intrinsics during compiling -->
<AdditionalOptions Condition="'$(Platform)' == 'x64' and $(PlatformToolset) == 'ClangCL' and '$(LLVMToolsVersion)' &lt; '19'">/arch:AVX</AdditionalOptions>
</ClCompile>
<ClCompile Include="..\Modules\_codecsmodule.c" />
<ClCompile Include="..\Modules\_collectionsmodule.c" />
Expand DownExpand Up@@ -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)' &lt; '19'">

@chris-eiblchris-eiblFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

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.

@zoobazoobaFeb 24, 2025

Copy link
Copy Markdown
Member

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.

@chris-eiblchris-eiblFeb 24, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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?

@zoobazoobaFeb 24, 2025

Copy link
Copy Markdown
Member

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.

@chris-eiblchris-eiblFeb 24, 2025

Copy link
Copy Markdown
MemberAuthor

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

<!-- GH-130213: blake2module.c needs to just "see" the AVX intrinsics during compiling.
But older clangs only make them visible, if they are compiling
for an architecture that supports them.
-->
<Warning Text="built executable requires AVX and may not run everywhere" />
</Target>
<Target Name="_WarnAboutZlib" BeforeTargets="PrepareForBuild" Condition="!$(IncludeExternals)">
<Warning Text="Not including zlib is not a supported configuration." />
</Target>
Expand Down