Fix x86 fib_recursive#39
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
force-pushed
the
x86-fib-recursive
branch
5 times, most recently
from
d510ad6 to
9c27bc8
Compare
…s/guards
assembler.rs:
- gen_shadowstack_header / gen_footer_shadowstack push/pop the
is_minor marker and rbp jf_ptr on JIT entry/exit.
- reload_frame_if_necessary reads jf_ptr from *(root_stack_top - WORD)
instead of chasing jf_forward.
- push_all_regs_to_jitframe / pop_all_regs_from_jitframe write managed
GPRs (and optionally XMMs) through the gcmap-indexed jitframe slots.
- CallMallocNurseryVarsizeFrame: inline bump-allocator fast path;
slowpath bracketed by push_all_regs / pop_all_regs, publishes
pending_malloc_nursery_gcmap, calls dynasm_nursery_slowpath_jitframe,
and follows with reload_frame_if_necessary.
- GcLoad: handle Loc::Immed base by staging through R11.
- cranelift slowpath: reload_frame_if_necessary after the helper call.
GcStore / Guard hardening:
- emit_op_gcstore_imm_regalloc: 64-bit immediate that does not fit imm32
is split into two 32-bit DWORD stores, removing the R11 / offset
collision with LARGE_IMM_SCRATCH.
- emit_op_gcstore_regalloc / emit_op_gcstore_imm_regalloc panic on
ofs_loc other than Loc::Reg or Loc::Immed.
- emit_gcstore_sized / emit_gcstore_imm_sized panic on store sizes
outside {1,2,4,8}.
- _cmp_guard_class and _cmp_guard_gc_type panic on unsupported obj_loc /
class_loc / typeid_loc shapes instead of silently emitting no CMP and
leaving the caller's CC_E branch on stale flags.
regalloc.rs:
- next_op_can_accept_cc / force_allocate_reg_or_cc let CompOp and
IntAddOvf leave their result in the condition flags when consumed by
the immediately following Guard{True,False,Nonnull,Isnull} or
CondCallN.
check.py:
- fib_recursive on dynasm: allow up to 10x cpython, timeout 15s.
genop_call_assembler now mirrors rpython/jit/backend/llsupport/assembler.py:295 `call_assembler` + rpython/jit/backend/x86/assembler.py:2267 `_call_assembler_emit_call` layout for the resolved-target hot path. - Drop the caller-side `mov r12, rbp; ... ; mov rbp, r12` wrapping. The callee _call_header pushes caller rbp and _call_footer pops it, so caller rbp is preserved across the call without a register save. - Drop the redundant `mov rdx, rax` rename — feed rax (the trampoline return value = callee jf_ptr) straight into the CMP and the fast-path result load. - Reduce the descr check from `mov rcx, [rdx+ofs]; mov rax, imm; cmp rcx, rax` (3 instr) to `mov R11, imm; cmp [rax+ofs], R11` (2 instr), matching PyPy's CMP with memory operand. fib_recursive 9.5s -> 8.9s (~6%, fib(35)).
assembler.rs: - Drop the call_assembler_execute_trampoline indirection in both call-paths of _call_assembler_emit_call / genop_call_assembler. The compiled callee entry address is loaded straight into rax and invoked, removing the second arg push and an extra indirect call per recursion. - Reflow several panic! macro invocations onto single lines (rustfmt). check.py: - fib_recursive dynasm gate restored to (5, 1.5, 10, 1.5, 10) from the previously widened (15, 10, None, 1.2, 8).
youknowone
force-pushed
the
x86-fib-recursive
branch
from
9c27bc8 to
11145dc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c27bc84ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dynasm!(self.mc ; .arch x64 ; mov QWORD [rbp + gcmap_ofs], 0); | ||
| self.reload_frame_if_necessary(); |
There was a problem hiding this comment.
In CallMallocNurseryVarsizeFrame slowpath, this writes JF_GCMAP_OFS to zero before reload_frame_if_necessary(). After a minor GC, rbp may still point to the old nursery copy, so this clears the stale frame while leaving the moved jitframe's gcmap published. That keeps an out-of-date gcmap live for subsequent collecting calls and can cause incorrect root tracing. Reload rbp first, then clear JF_GCMAP_OFS on the current frame (as done in pop_pending_call_gcmap_after_collect).
Useful? React with 👍 / 👎.
| | OpCode::GuardFalse | ||
| | OpCode::GuardNonnull | ||
| | OpCode::GuardIsnull | ||
| | OpCode::CondCallN |
There was a problem hiding this comment.
Allowing CondCallN in next_op_can_accept_cc lets comparisons keep their result only in condition codes (frame_reg sentinel path), but the CondCallN emitter still evaluates arg0 as a materialized boolean (genop_discard_cond_call does test on the value) and does not consume guard_success_cc. This can skip or execute conditional calls based on stale/non-materialized data when a compare is immediately followed by CondCallN.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
x86-fib-recursive
branch
from
11145dc to
e95baaf
Compare
In CallMallocNurseryVarsizeFrame's nursery-overflow slowpath, swap the order so reload_frame_if_necessary runs before the mov QWORD [rbp + JF_GCMAP_OFS], 0 clear. Matches the upstream _build_malloc_slowpath pattern (x86/assembler.py:295-308): CALL, _reload_frame_if_necessary, pop_gcmap. Without the reorder, if a minor GC during the slowpath copied the jitframe to old-gen, the clear targeted the freed nursery copy and left the moved frame's gcmap published, which a later collecting call would walk. Also reword the next_op_can_accept_cc docstring to cross-reference genop_discard_cond_call for the COND_CALL CC-fusion path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0639b2d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -6024,6 +6360,12 @@ impl<'a> Assembler386<'a> { | |||
| self.emit_abi_call_rax(); | |||
| // pop_gcmap | |||
| dynasm!(self.mc ; .arch x64 ; mov QWORD [rbp + gcmap_ofs], 0); | |||
There was a problem hiding this comment.
In genop_call_malloc_nursery slowpath, JF_GCMAP_OFS is zeroed before reload_frame_if_necessary(). If the helper triggers a minor GC that moves the jitframe, rbp still points to the old nursery copy at this point, so the clear hits stale memory and the moved frame keeps a published gcmap. That stale gcmap can then be consumed by later collecting calls and produce incorrect root tracing; this path needs the same reload-then-clear ordering used elsewhere in this patch.
Useful? React with 👍 / 👎.
1 participant
Fix #35
Summary
Self-review
Prompt & Model
Model:
Prompt:
Answer