GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-130213: clang-cl: blake2module.c needs to "see" the AVX intrinsics during compiling by chris-eibl · Pull Request #130447 · python/cpython

Conversation

@chris-eibl

@chris-eibl chris-eibl commented Feb 22, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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?

  • Issue: blake2module.c needs to be compiled with libhacl SIMD flags #130213

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

msprotz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@chris-eibl
pythonGH-130213: blake2module.c needs to "see" the AVX intrinsics dur…
3f110a5
…ing compiling
@chris-eibl
chris-eibl requested a review from a team as a code owner February 22, 2025 07:52
@bedevere-app bedevere-app Bot added the awaiting review label Feb 22, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 22, 2025
blake2module.c needs to be compiled with libhacl SIMD flags #130213
Closed
chris-eibl
chris-eibl commented Feb 22, 2025
View reviewed changes
Comment thread PCbuild/pythoncore.vcxproj
<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-eibl chris-eibl Feb 22, 2025

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@picnixz picnixz added the skip news label Feb 22, 2025
gpshead
gpshead approved these changes Feb 22, 2025
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 22, 2025
@chris-eibl
chris-eibl marked this pull request as draft February 23, 2025 12:35
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 23, 2025
@chris-eibl

chris-eibl commented Feb 23, 2025

Copy link
Copy Markdown
Member Author

Converted to draft because not needed in case #130483 works out

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba
zooba reviewed Feb 24, 2025
View reviewed changes
Comment thread PCbuild/pythoncore.vcxproj
<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'">

@zooba zooba Feb 24, 2025

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@chris-eibl chris-eibl Feb 24, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@zooba zooba Feb 24, 2025

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chris-eibl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@chris-eibl chris-eibl Feb 24, 2025

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@chris-eibl chris-eibl mentioned this pull request Feb 24, 2025
gh-130213: POC let blake2module.c see far less internals #130483
Closed
@chris-eibl chris-eibl mentioned this pull request Mar 9, 2025
GH-130090: build time analysis #131005
Closed
@chris-eibl

chris-eibl commented Mar 11, 2025

Copy link
Copy Markdown
Member Author

Closed since superseded by #130960.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@chris-eibl chris-eibl closed this Mar 11, 2025
@chris-eibl chris-eibl mentioned this pull request Mar 15, 2025
gh-130213: update hacl_star_rev to 322f6d58290e0ed7f4ecb84fcce12917aa0f594b #130960
Merged
@chris-eibl
chris-eibl deleted the clang-cl_fix_for_blake2module.c branch March 15, 2025 19:48
@chris-eibl chris-eibl mentioned this pull request Mar 24, 2025
GH-131521: fix clangcl build on Windows for zlib-ng #131526
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments
@gpshead gpshead gpshead approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@chris-eibl @gpshead @zooba @picnixz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.