◐ Shell
reader mode source ↗
Skip to content

GH-131521: fix clangcl build on Windows for zlib-ng#131526

Merged
zooba merged 7 commits into
python:mainfrom
chris-eibl:fix_clangcl_zlibng
Mar 24, 2025
Merged

GH-131521: fix clangcl build on Windows for zlib-ng#131526
zooba merged 7 commits into
python:mainfrom
chris-eibl:fix_clangcl_zlibng

Conversation

@chris-eibl

@chris-eibl chris-eibl commented Mar 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I gave it a try. For me it compiles and test_zlib is green.

I've only thrown in as many -m as needed.

But only for those files that really need them!

I started to set that for all files (the diff would have been much smaller), but

  • this does not seem right to me: the compiler would be eligible to generate such code for all *.c files given, and the resulting binary would then not run on all hosts
  • test_zlib failed with invalid instruction if I compiled all *.c files with the "all the -m. This convinced me even more to be "selective" here: my ancient Haswell i5 4570 only supports up to AVX2 and it makes sense, that it would not happily accept all that instructions (up to AVX512).

@chris-eibl chris-eibl requested a review from a team as a code owner March 20, 2025 20:13
@bedevere-app bedevere-app Bot mentioned this pull request Mar 20, 2025
Closed
@chris-eibl

chris-eibl commented Mar 20, 2025

Copy link
Copy Markdown
Member Author

Maybe someone can trigger Windows tailcall CI - it's the only one that really is of interest here, but is only triggered for ceval and the like

cpython/.github/workflows/tail-call.yml

Lines 4 to 9 in ce79274

paths:
- '.github/workflows/tail-call.yml'
- 'Python/bytecodes.c'
- 'Python/ceval.c'
- 'Python/ceval_macros.h'
- 'Python/generated_cases.c.h'

@zooba

zooba commented Mar 20, 2025

Copy link
Copy Markdown
Member

Maybe someone can trigger Windows tailcall CI

Just touch one of those files - a comment will do. And undo it before it gets merged (make the comment something like // DO NOT MERGE - just triggering CI)

@chris-eibl
use -msse4.2 for adler32_sse42.c
335009b
@chris-eibl
Do not use AdvancedVectorExtensions2 for all *.c files, …
4bdf82f 
so that the resulting binary can be executed on older CPUs, too.
Also use AdvancedVectorExtensions512 where necessary.
@chris-eibl
Merge branch 'main' into fix_clangcl_zlibng, because …
3ad6eb2 
some of the files tail-call.yml is watching were touched
on main, and thus I think it will fire :)
@chris-eibl
// DO NOT MERGE - just triggering Windows tail-call CI)
b651937
@chris-eibl chris-eibl requested a review from markshannon as a code owner March 22, 2025 07:52
@chris-eibl

chris-eibl commented Mar 22, 2025

Copy link
Copy Markdown
Member Author

I think this is a skip news?

@chris-eibl

chris-eibl commented Mar 22, 2025

Copy link
Copy Markdown
Member Author

Windows tail-call CI is now green: https://github.com/python/cpython/actions/runs/14006628024/job/39221297660

Having a detailed look, ISTM that the tests are not run, though. This is not related to this PR, the last green build some days ago did not run them either: https://github.com/python/cpython/actions/runs/13954576580/job/39062383429#step:4:326

After building, no more output can be seen.

This is interesting, because

cpython/.github/workflows/tail-call.yml

Lines 82 to 91 in ce79274

- name: Native Windows (debug)
if: runner.os == 'Windows' && matrix.architecture != 'ARM64'
shell: cmd
run: |
choco install llvm --allow-downgrade --no-progress --version ${{ matrix.llvm }}.1.5
set PlatformToolset=clangcl
set LLVMToolsVersion=${{ matrix.llvm }}.1.5
set LLVMInstallDir=C:\Program Files\LLVM
./PCbuild/build.bat --tail-call-interp -d -p ${{ matrix.architecture }}
./PCbuild/rt.bat -d -p ${{ matrix.architecture }} -q --multiprocess 0 --timeout 4500 --verbose2 --verbose3

clearly ./PCbuild/rt.bat is invoked. Maybe the reason is, because it is not used with backslashes like in

cpython/.github/workflows/reusable-windows.yml

Line 52 in ce79274

.\\PCbuild\\rt.bat

The question then is, why ./PCbuild/build.bat is working ...

@Fidget-Spinner: any idea?

@Fidget-Spinner

Fidget-Spinner commented Mar 22, 2025

Copy link
Copy Markdown
Member

CI uses cmd instead if powershell, could that make a difference?

@chris-eibl

chris-eibl commented Mar 22, 2025

Copy link
Copy Markdown
Member Author

Maybe?
IMHO, we shouldn't do that here in this PR but create a separate issue and PR?

@zooba

zooba commented Mar 24, 2025

Copy link
Copy Markdown
Member

CI uses cmd instead if powershell, could that make a difference?

It probably requires call PCbuild\build.bat ... to handle someone calling exit in the batch file. In Cmd, if the entire block is a batch file, then exit will break out of the top-level one, not the nested one, unless you use call.

@chris-eibl chris-eibl mentioned this pull request Mar 24, 2025
Closed
@chris-eibl

chris-eibl commented Mar 24, 2025

Copy link
Copy Markdown
Member Author

CI uses cmd instead if powershell, could that make a difference?

It probably requires call PCbuild\build.bat ... to handle someone calling exit in the batch file. In Cmd, if the entire block is a batch file, then exit will break out of the top-level one, not the nested one, unless you use call.

I've created #131678 to continue the discussion there.

@zooba zooba merged commit d16f455 into python:main Mar 24, 2025
@chris-eibl chris-eibl deleted the fix_clangcl_zlibng branch March 24, 2025 19:29
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 1, 2025
@chris-eibl @diegorusso
pythonGH-131521: Uses correct build flags for zlib-ng on Windows (pyt… …
64dac61 
…honGH-131526)

Do not enable AdvancedVectorExtensions2 for all *.c files, so that the resulting binary can be executed on older CPUs, too. Also enable AdvancedVectorExtensions512 where necessary, and add the ClangCL flags required to enable vector extensions.
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

@markshannon markshannon Awaiting requested review from markshannon

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chris-eibl @zooba @Fidget-Spinner