GH-136895: Update JIT builds to use LLVM 20 #140329
Conversation
With LLVM 20, individual files are greater than the 100MiB single file limit for items checked into git. Therefore, this PR pulls down binaries from GitHub releases, as `.tar.xz` files to additionally maximize compression ratio. Currently this is somewhat of a first draft, as there are things like hash checking needed to be done.
This reverts commit e6450de.
savannahostrowski
requested review from
a team,
brandtbucher and
diegorusso
as code owners
diegorusso
left a comment
diegorusso
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the comments.
|
Trying to do my due diligence here before this gets merged... @python/windows-team Does someone want to have a look at the changes in |
emmatyping
left a comment
emmatyping
left a comment
There was a problem hiding this comment.
Two suggestions, but I think get_externals[.py,.bat] look good otherwise!
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
A bit of C pedantry. Otherwise looks good.
|
When you're done making the requested changes, leave the comment: |
brandtbucher
left a comment
brandtbucher
left a comment
There was a problem hiding this comment.
Awesome! Here's a partial review of everything except the stuff in Tools (I'd like to dig a bit more into the changes to the flags and relocations and see how they affect the code quality).
Python/jit.c
Outdated
| /* Generate the trampoline (14 bytes, padded to 16): | ||
| 0: ff 25 00 00 00 00 jmp *(%rip) | ||
| 6: XX XX XX XX XX XX XX XX (64-bit target address) | ||
| Reference: https://wiki.osdev.org/X86-64_Instruction_Encoding#FF (JMP r/m64) | ||
| */ | ||
| trampoline[0] = 0xFF; | ||
| trampoline[1] = 0x25; | ||
| *(uint32_t *)(trampoline + 2) = 0; | ||
| *(uint64_t *)(trampoline + 6) = value; |
There was a problem hiding this comment.
@savannahostrowski... just want to make sure it's not lost on you how badass you are hand-writing x86-64 machine code byte-by-byte like this. 🔥
emmatyping
left a comment
emmatyping
left a comment
There was a problem hiding this comment.
This looks great! Very excited for this
|
I am going to merge this since it has multiple reviews and is blocking some other work that we'd like to do. @brandtbucher let's chat offline about any other changes we'd like to make here re. flags/relocations, happy to follow up! |
8 participants
Alright, this took me longer than expected for two reasons:
Bumping to LLVM 20 required some changes to the infrastructure we use to grab LLVM as a dependency on Windows. As of 20, LLVM now contains files that exceed GitHub's allowable size, so checking these directly into
cpython-bin-deps' tree was not an option. Instead, this PR adds the ability to pull from release artifacts (see https://github.com/python/cpython-bin-deps/releases/tag/llvm-20.1.8.0). This also makes the process of bumping LLVM for Windows a little less hairy. Thank you, @emmatyping, for much of this code, and thank you, @zware, for publishing the release artifact for me 🙏 !For macOS x86_64 debug builds, external symbol references generated by LLVM 20 can exceed the ±2GB PC-relative addressing range that
patch_32rrequires. This manifested as assertion failures in free-threading tests (see https://github.com/savannahostrowski/cpython/actions/runs/18438327725/job/52537027963?pr=10 as an example). After going down a rabbit hole, I believe the correct fix here is to implement x86_64 trampolines (similar to our existing aarch64 implementation) to handle out-of-range symbols...and so I've done that. AFAICT, trampolines are only needed on macOS right now, as the other target platforms have different relocation mechanisms.