removing skip_oss and is_oss to force tests to run - #21
Closed
grantlouisherman wants to merge 43 commits into
Closed
removing skip_oss and is_oss to force tests to run#21grantlouisherman wants to merge 43 commits into
grantlouisherman wants to merge 43 commits into
Conversation
meta-cla
Bot
added
the
CLA Signed
grantlouisherman
marked this pull request as draft
April 7, 2026 17:40
Summary: Added a postgen pass that identifies Move instructions with 64-bit immediate values needing >2 movz/movk instructions. When the same large constant appears more than once, rewrites to kMovConstPool which loads via PC-relative ldr from a constant pool emitted after deopt exits. Saves code size for repeated large constants. New files touched: instruction.h (kMovConstPool opcode), environ.h (pool tracking), postgen.cpp (rewrite pass), autogen.cpp (translateMovConstPool + rules), gen_asm.cpp (pool emission). Reviewed By: alexmalyshev Differential Revision: D99198077 fbshipit-source-id: da1b3b9dcbf4d248f0809c802d223131fb7021a8
Summary: We don't support this. Reviewed By: yoney Differential Revision: D99895133 fbshipit-source-id: 4a745b00b01c99c22fc963159296d97546c55484
Reviewed By: jbower-fb Differential Revision: D99895253 fbshipit-source-id: 17ed786da77e938f5014d31d10cd67c23de738ef
Summary: Add `specialize_py_call` and `specialize_py_call_kw` to tsan suppression list. Reviewed By: jbower-fb Differential Revision: D99887013 fbshipit-source-id: a39a26373cd9a720283cd8d3ee5e0e06d90c2012
Summary: There's a bug in the register allocator exposed by D97879718 that shows up only in an instagram canary. When we're performing register allocation for a function with a large number of parameters we can end up clobbering the function register. We produce assembly that looks like: ``` 0x007fd6040005ec: movq %rbx, -0x130(%rbp) 0x007fd6040005f3: movq %rax, %rbx 0x007fd6040005f6: movq %fs:0xffffffffffffffe8, %rax 0x007fd6040005ff: movq %rdi, -0x138(%rbp) <— spilling function 0x007fd604000606: leaq -0xf0(%rbp), %rdi 0x007fd60400060d: movq %r8, -8(%rdi) <— no load, storing garbage before the frame 0x007fd604000611: movq %r10, -0x140(%rbp) 0x007fd604000618: movl (%r8), %r10d 0x007fd60400061b: incl %r10d 0x007fd60400061e: je 0x7fd604000627 0x007fd604000624: movl %r10d, (%r8) 0x007fd604000627: movabsq $0x7fd602137a30, %r8 0x007fd604000631: movq %r8, (%rdi) ``` When we should produce: ``` 0x007fa7104005ec: movq %rbx, -0x130(%rbp) #312 0x007fa7104005f3: movq %rax, %rbx 0x007fa7104005f6: movq %fs:0xffffffffffffffe8, %rax 0x007fa7104005ff: movq %rdi, -0x138(%rbp) <— spilling function 0x007fa710400606: leaq -0xf0(%rbp), %rdi 0x007fa71040060d: movq %r8, -0x140(%rbp) <— spilling r8 0x007fa710400614: movq -0x138(%rbp), %r8 <— loading r8 that was spilled 0x007fa71040061b: movq %r8, -8(%rdi) <-- storing function before frame 0x007fa71040061f: movq %r10, -0x148(%rbp) 0x007fa710400626: movl (%r8), %r10d 0x007fa710400629: incl %r10d ``` Claude was able to identify the bug in the register allocator where when the ends are the same we free the value from the stack. The diff before and after when running with PYTHONJITDEBUGREGALLOC=1 looks like: ``` JIT: Allocating blocked location R8 to interval [47, 58) JIT: Split new interval ->R8: [47, 105) -JIT: Reusing stack slot [RBP(-312)] for operand %8:Object -JIT: Allocating blocked location [RBP(-312)] to interval ->R8: [47, 105) +JIT: Allocating new stack slot [RBP(-320)] for operand %8:Object +JIT: Allocating blocked location [RBP(-320)] to interval ->R8: [47, 105) JIT: Allocating blocked location R10D to interval [51, 58) JIT: Split new interval ->R10: [51, 105) -JIT: Allocating new stack slot [RBP(-320)] for operand %22:Object ``` Reviewed By: jbower-fb Differential Revision: D99716864 fbshipit-source-id: 91a68b020774a4b7853455f88de3ee4863ae530d
Summary: Add a minimal top-level guard for JIT entrypoints in free-threaded builds. The goal is to make the current FT test path reliable by serializing top-level registration, scheduling, compilation, and teardown paths that can now race without the GIL. A more integrated alternative would be to extend `ThreadedCompileSerialize` to cover these FT entrypoints too, but that is a larger change. In particular, `preload` can re-enter other JIT paths, so folding it into the existing threaded-compile locking model needs more restructuring to avoid lock-ordering problems. This change is FT-specific and should not change GIL builds. In non-FT builds the new guard is a no-op and should optimize away. Reviewed By: jbower-fb, DinoV Differential Revision: D99853055 fbshipit-source-id: 546f122605b307337f2295fc2c43527d8ee2dfd0
Summary: When `bl(imm)` is used on ARM64 with an absolute address, asmjit now selects between a direct `bl` (4 bytes, ±128MB range) and an indirect `ldr x16, [pc+off]; blr x16` (8 bytes, unlimited range via address table), similar to how x86-64 handles `call(imm)` with the kX64AddressEntry mechanism. Reviewed By: alexmalyshev Differential Revision: D98746485 fbshipit-source-id: 7709325a505a1564d0b26d958b596c9f032cf64e
Summary: Take advantage of the new ARM64 branch patching in asmjit to directly branch when possible. Reviewed By: alexmalyshev Differential Revision: D98746483 fbshipit-source-id: 4e6d24982b32bd76e58da1c9e6cd1df73223c724
Summary: Add branch patching for tbz/tbnz for asmjit so that it will attempt to use these instructions if they fit, and otherwise fall back to a longer form. Reviewed By: alexmalyshev Differential Revision: D98746480 fbshipit-source-id: 4c3b5096d2d79cee3186d1592aee6f5ef5e8aad6
Summary: Take advantage of the new ARM64 tbnz branch patching. Reviewed By: alexmalyshev Differential Revision: D98746486 fbshipit-source-id: b20ce4dcfae3090af6e29d714e9b3e554b4c72a6
Summary: Let asmjit use cbz/cbnz without worrying about range, as it will now fall back to a larger version. Reviewed By: alexmalyshev Differential Revision: D98746481 fbshipit-source-id: a37e0104d6a165b6c0219037787dc1d6337eecd0
Summary: Take advantage of the new ARM64 cbnz branch patching. Reviewed By: alexmalyshev Differential Revision: D98746482 fbshipit-source-id: f50fecea46500213db5c8a7ce26828c6d153203a
Summary: Support patching b.cond and adr in ARM64 in asmjit when the location is out of range. Reviewed By: alexmalyshev Differential Revision: D98746477 fbshipit-source-id: ecf2cfc72a612b14175ae5b2c87d164104ee0111
Summary: Extend the existing bl(imm) relaxation mechanism to also handle b(imm). When the 26-bit displacement (±128MB) doesn't fit, the instruction is relaxed to `ldr x16, [pc+off]; br x16` via the address table, analogous to `ldr x16, [pc+off]; blr x16` for bl. Reviewed By: alexmalyshev Differential Revision: D98746478 fbshipit-source-id: 6bc3ce384ef3a6415c7a05690d37a228cc165728
Summary: Frame reification via getCodeRuntime can look up JIT compilation state (`lookupCodeRuntime` -> `lookupFunc` -> `lookupCode`) without entering through any guarded top-level JIT entrypoint. `FreeThreadedJITEntrypointGuard` is no-op for GIL build. ThreadSanitizer: data race third-party/parallel-hashmap/parallel_hashmap/phmap.h:2307 in phmap::priv::raw_hash_set<phmap::priv::FlatHashMapPolicy<jit::CompilationKey, BorrowedRef<jit::CompiledFunction>>, std::hash<jit::CompilationKey>, std::equal_to<jit::CompilationKey>, std::allocator<std::pai r<jit::CompilationKey const, BorrowedRef<jit::CompiledFunction>>>>::set_ctrl(unsigned long, signed char) ================== ``` Reviewed By: DinoV Differential Revision: D100043553 fbshipit-source-id: 0b98081819bf0905b2b5dfcb82189be07b865313
Summary: In free-threaded builds, the JIT can modify a function's vectorcall field concurrently with the interpreter reading it. Wrap `vectorcall == _PyFunction_Vectorcall` checks with `FT_ATOMIC_LOAD_PTR_RELAXED`. ThreadSanitizer: data race fbcode/cinderx/Interpreter/3.14/Includes/generated_cases.c.h:1577 in Ci_EvalFrame ================== ```` Reviewed By: DinoV Differential Revision: D100068060 fbshipit-source-id: e9a05bd28314122a360a500213a26fee18da0b9b
Summary: In the face of inlined methods we're not tracking the deopt index the same as we do on x64. This adds tracking for the inline methods so we can have the whole information. Reviewed By: yoney Differential Revision: D99380230 fbshipit-source-id: 5b73e28afd8b7a6e723297a8160982cb3df8b675
Summary: Move interpreter frame allocation and linking out of the hand-written assembly in frame_asm.cpp into LIR generation (generator.cpp), letting the register allocator manage registers across the frame setup. Key changes: - Add emitLoadFrame() in generator.cpp (~300 lines) handling generators, normal frames, lightweight frames, and 3.10 shadow frames - Add kLoadThreadState LIR instruction with TLS fast path for loading the thread state pointer, with register reservation in regalloc.cpp - Change callee-saved register save/restore from SP-relative push/pop to FP-relative fixed-offset mov/stp/ldp in gen_asm.cpp - Emit LoadFrame HIR for all Python versions (3.10+), not just 3.12+ - Add spill copy (memcpy) in JITRT_AllocateAndLinkGenAndInterpreterFrame to preserve spilled register values when swapping to generator storage - Delete frame_asm.cpp/h (1093+112 lines) and register_preserver.cpp/h (198+45 lines) along with their BUCK targets and tests - Update expected HIR in 20 test files to include LoadFrame Reviewed By: alexmalyshev Differential Revision: D97879718 fbshipit-source-id: 9cb3246755941bddbbe15bbdc1b370d80e745d18
Summary: dev builds are currently blowing up on startup, this fixes it. Reviewed By: yoney Differential Revision: D100082864 fbshipit-source-id: 7df7a2daa44ed69164415657a6e25ec29d2ce327
Summary: While looking at the disassembly in D98820285 I noticed we weren't getting our TLS optimization on ARM. The bug was a typo in the mrs instruction comparison constant on line 41. 0xd53bd048 should have been 0xd53bd040. The & ~0x1f mask zeroes the bottom 5 register-encoding bits, but the constant 0xd53bd048 still had bit 3 set (encoding register x8), making (masked_value == 0xd53bd048) always false. The compiler proved this at compile time and eliminated the entire detection block as dead code. Reviewed By: martindemello Differential Revision: D98932574 fbshipit-source-id: f859b3d5dcb1b4e0f0ce3108d892c10b8077d473
Summary: Hot/cold splitting is actually resulting in lots of InvalidDisplacement errors that you'll see if you run with `PYTHONJITDEBUG=1 PYTHONJITALL=1` so methods just aren't JITing. And this shows up in `test_max_code_size_slow` in particular because we fail to reach the maximum code size. This updates the allocator so that we use pages within range for both hot and cold pages. Reviewed By: kddnewton Differential Revision: D99506288 fbshipit-source-id: 3df6fe9db8a97b35a63078e89ade2b72556fa714
Summary: Spawning processes in QEMU can be slow, let's provide extra time. Reviewed By: yoney Differential Revision: D99514824 fbshipit-source-id: bd864041d21df2deb6d96087e8277c30aa747e76
Summary: We get little value from running these tests on multiple platforms anyway as they exercise pure-python code. But they're already long running so let's skip them on cross platform runs. Reviewed By: yoney Differential Revision: D99870800 fbshipit-source-id: 7c749d13bba62fabdaaea9912332db8112bd253c
Summary: Add experimental support for free-threaded Python. Reviewed By: grantlouisherman Differential Revision: D99869989 fbshipit-source-id: 1d99132841a7758c2f808a0a6176412c46dcffe1
Summary: Remove the unused `tstate_reg` parameter from `saveCallerRegisters` and `setupFrameAndSaveCallerRegisters`. The parameter was already marked `[[maybe_unused]]` and not referenced in either function body. Reviewed By: yoney Differential Revision: D98265342 fbshipit-source-id: cdf008c7cba52d50904d74e9fc97a23dca260b2f
Summary: On normal function entry we currently are splitting our saving of stack space and callee preserved registers. This unifies it to happen in a single place - and it's now happening where the static entry point enters to so we don't duplicate it in our entry code. Reviewed By: yoney Differential Revision: D98267991 fbshipit-source-id: 7dd45c4fac70a96d6cc85fbf69eeb8f06c9a7819
Summary: Replace the hand-written generateResumeEntry() asmjit code with a post-regalloc LIR basic block that uses two new instructions: - kSetupFrame: emits generateFunctionEntry + setupFrameAndSaveCallerRegisters using frame layout from Environ, and binds gen_resume_entry_label - kIndirectJump: emits jmp [base+offset] on x86 / ldr+br on aarch64 The gen-data-footer manipulation (loading gi_jit_data, saving link/return addresses, switching the frame pointer, loading/clearing yieldPoint) is expressed as regular kMove instructions with physical register operands. The resume block is appended to the LIR function after register allocation but before autogen runs, so it participates in normal code emission without needing regalloc (all operands are physical registers). Reviewed By: alexmalyshev Differential Revision: D98354276 fbshipit-source-id: a54bfa086178bc46f2698ee60ba83beae242c255
Summary: Also adds a generic `skip_if_ft()` decorator. Reviewed By: yoney Differential Revision: D99959456 fbshipit-source-id: 7d76fd2e0dfe874b9c75afe0009c570ee496ccf6
Summary: `openssl@1.1` is no longer included in Homebrew, in favor of `opennssl@3`. Reviewed By: iahs Differential Revision: D100210666 fbshipit-source-id: eb871b11b4f7a5e85eb6ae7cce57f9627ac115e3
Summary: doeswhatitsaysonthetin Reviewed By: DinoV Differential Revision: D99959454 fbshipit-source-id: ca218f422b71873a0019a06a4090b3e0da02acd2
Summary: Use identity comparison (`is`) instead of equality (`==`) when checking if an object is in gc.get_objects(). In 3.14t, gc.get_objects() includes a FoldedCase object from importlib.metadata whose __eq__ calls other.lower(), which crashes on objects without that attribute. Reviewed By: yoney Differential Revision: D100258864 fbshipit-source-id: ac9b64e104ee8dbd4d9a2e07eafdab82b97c4a06
Summary: Initializing this in the entry block can clobber the argument registers because it happens before they're populated via kLoadArg. We already initialize it for generators in here so we should consistently initialize it for everything. Reviewed By: yoney Differential Revision: D100256832 fbshipit-source-id: 370879dd912264d2730773f3a42eb0f923a78d03
Summary: We're failing to generate our deopt trampolines on some specific architectures or builds: https://fb.workplace.com/groups/248741357802476/permalink/940368701973068/ This is removing the newly introduced use of `bl` on startup to avoid that. Reviewed By: kddnewton Differential Revision: D100362498 fbshipit-source-id: 73903a383e986f78af88456286c25a4ab018c6bc
Summary: This diff reverts D97536461 This introduces a memory leak in the JIT Depends on D97536461 Reviewed By: DinoV Differential Revision: D98552127 fbshipit-source-id: bbbbdb9a23d4835c0d71297dee53a2e0075edbfd
Summary: Currently we emit push and pop to transfer registers on both ARM and x64. But on ARM we have a scratch register that we can use instead which will be much more efficient so this switches to using that. Ideally we'd get rid of the scratch register but we also still need it for xExchange which is also used in here so this isn't making things worse. Reviewed By: yoney Differential Revision: D100222370 fbshipit-source-id: f923c2338efe75d36a8f7db70531857822ea92b7
Summary: Shipit uses `rebalancer` as project than `facebookincubator/rebalancer` ``` + /var/www/scripts/opensource/codesync \ + shipit \ + --project=rebalancer \ + --create-new-repo \ + --source-repo-dir=/data/users/sahilsd/fbsource \ + --source-branch=. \ + --skip-source-init \ + --skip-source-pull \ + --skip-source-clean \ + --skip-push \ + --destination-use-anonymous-https \ + --create-new-repo-output-path=/data/users/sahilsd/scratch/getdeps/shipit/rebalancer.new error: invalid value 'rebalancer' for '--project <PROJECT>': Valid projects are specified in the form of '<organization>/<repo_name>', given: rebalancer ``` This matches with the shipit config comment: https://www.internalfb.com/code/configerator/[8f710a959bf55ae647eb35bcacf3322bbd2ccba2]/source/opensource/shipit_config/facebookincubator/rebalancer.cconf?lines=1 Reviewed By: r-barnes Differential Revision: D97680878 fbshipit-source-id: 077331584a3e1d58067ca62706efe437e042c36d
Summary: Users can still do `import cinderx`, but they'll get the stub definitions. Reviewed By: DinoV Differential Revision: D98516932 fbshipit-source-id: 89bd0099863f3bbdf5879bebd12ae6882fa0671f
Summary:
Deopt the frames currently on the stack (on all threads) when instrumentation (profiling, tracing, debugging) activates. This is done by walking the stacks in each thread and changing their return address to the deopt address.
**How this change works:**
**Code Generation Stage:**
- In `TranslateGuard` we take advantage of the debug loc added by `ADDDEBUGENTRY` for the `kCall` instruction to store a grouping of {return label address, deopt label address}
- In `finalizecode()` we make the same mapping but with the actual memory addresses (return address -> deopt address)
**Deopt Patching Logic:**
- `toggleJitBasedOnInstrumentationState()`: Called when instrumentation activates. Now calls `deoptAllJitFramesOnStack()` as well
- `deoptAllJitFramesOnStack()`:
- Skips patching the topmost frame (literally the call activating instrumentation like `sys.monitoring.register_callback`)
- Skip patching inlined frames (already handled properly by deopt mechanism)
- Set `prev_instr`/`instr_ptr` in case something tries to read the frame between this patching and the actual deopt (e.g. `sys._getframe()`, tracebacks)
- Note: We save the innermost inlined frame in order to use that for `updatePrevInstr` (so it can walk backward and set `prev_instr`/`instr_ptr` for the outer and inlined frames)
- Populate frame, overwrite return address, set `JIT_FRAME_DEOPT_PATCHED` in header
- `setIP()`: Sets the return address of a frame (reverse of `getIP()`)
- `jitgen_am_send()`: If JIT is paused then deopt generator (this function is called when a generator/coroutine is about to be resumed)
**Deopt Trampoline:**
- `prepareForDeopt()`:
- Check if it’s an instrumentation deopt. If so we make sure to tell reifyFrame that
- Skip certain flows (e.g. `profileDeopt`) since this is a unique case of a deopt not happening frame an error. For the same reason, we also push return value
- `reifyFrameImpl()`:
- In 3.14 set `instr_ptr` to the next instruction (+1) since we don’t want to retry the call
- In 3.12 we don’t need to do anything different since we already point to the `CALL`’s inline cache area (so we’re set up for the next instruction properly)
- `resumeInInterpreter()`:
- We set an error if there was an error in addition to instrumentation deopt
- Set `f_trace_lines` and `f_trace` - something that CPython normally iterates through frames on the stack and does when it hits `RESUME` opcode, but since JIT deopts don’t hit that opcode we manually do it.
Note:
We use `tls_is_instrumentation_deopt` to carry over information from `prepareForDeopt` (where `FrameHeader` is still accessible and we can check `JIT_FRAME_DEOPT_PATCHED` to see if this is a frame that needs to be deopted) to resumeInInterpreter.
Note: Safety in the free-threaded build is done by using Stop The World semantics in `deoptAllJitFramesOnStack()`.
Reviewed By: alexmalyshev
Differential Revision: D93867884
fbshipit-source-id: 7307512c557303d4ff90c7c77e994813295eef0a
Summary: Having the mixed float/int operations forces us to use PyNumber runtime helpers which are very dynamic. If we know we have a float and a long, then we can convert the long to a float and kick off a FloatBinaryOp cheaply. The trick is how to do the long->float conversion cheaply. Add the ability to check for compact ints, which can be unboxed and converted to floats without a chance of generating a Python error. Also adds support for IntConvert to convert CInt64 values to CDouble values. Most integers are going to be compact, so this is an easy thing to add a guard for. We _could_ build this into the HIR type system, but many integer operations are going to widen CompactLong up to Long. It would only be useful for bitwise operations and unary negate. Don't try to simplify "Long BinaryOp Long" with compact checks. That gets unwieldy fast because the end of the operation is always a PrimitiveBox. We can simplify around the box in some cases, but it can't be DCE'd because it's needed for subsequent deopt paths. So we still incur the cost of the runtime helper no matter what. Reviewed By: DinoV Differential Revision: D99671044 fbshipit-source-id: 1d78cbaecf720b177ea2b8b14d3bc50d4aba95c7
Summary: 1. Add missed closeable Extension and re-enable the thrift.python.streaming package in setup.py. The streaming package was disabled and the closeable.pyx Cython extension was never registered, which caused ModuleNotFoundError at import time. 2. Remove the unconditional enable_tests=ON from [cmake.defines.os=linux] in the fbthrift-python manifest. This override forced tests on during dependency builds even when --no-tests was passed, which triggered a find_package(benchmark) failure. Add benchmark as an explicit dependency so test builds resolve correctly. 3. Remove test_queue_timeout from client_server tests. The test is architecturally incompatible with thrift-python's async handler model: combinedHandler calls create_task() and returns instantly, so the C++ executor dequeues all requests and calls tryStartProcessing() before the 10ms queue timeout timer fires. No blocking in the Python handler can cause requests to sit in the C++ resource pool queue long enough for the timeout to trigger. 4. Fix test_flag_behavior_matrix for stdlib Flag on OSS Python 3.12. Python stdlib Flag.__contains__ does not support `0 in FlagType` on all Python 3.12 builds. Skip the 0-containment check for stdlib PyFlag subclasses — only test it for thrift-python Flag types. 5. Remove unused SocketAddress import from client_server.py (was only used by the deleted test_queue_timeout). Reviewed By: vitaut Differential Revision: D99921093 fbshipit-source-id: 82dd9f939b7617d0ed244088bea020178ee9f3ca
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
No description provided.