-
Notifications
You must be signed in to change notification settings - Fork 19
majit: witness the undeclared field's width, and describe an array field as the pointer it is #1241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
860f0b0
3ce19d6
934057e
19f1b75
4dd5de9
b188874
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,196 @@ | ||
| /// jump.py: Frame layout remapping for register/frame moves. | ||
| /// | ||
| /// remap_frame_layout(assembler, src_locs, dst_locs, tmpreg) | ||
| /// — emit code to move values from src locations to dst locations, | ||
| /// handling overlaps via a temporary register. | ||
| //! `jump.py` — parallel assignment between location sets. | ||
| //! | ||
| //! Moving a set of values into a set of destinations is not a sequence of | ||
| //! independent moves: a destination may still be some other move's source, so | ||
| //! an order has to be found, and when the dependency graph has a cycle no | ||
| //! order exists and one value must be parked. `remap_frame_layout` is that | ||
| //! algorithm (`jump.py:4-64`), and `remap_frame_layout_mixed` (`jump.py:67-97`) | ||
| //! is the two-register-class variant used when integer and float arguments are | ||
| //! remapped together. | ||
| //! | ||
| //! ⚠ `dst_locations` and `src_locations` are swapped relative to upstream, | ||
| //! which spells the call `remap_frame_layout(assembler, dst_locations, | ||
| //! src_locations, tmpreg)`. Both are `&[Loc]`, so nothing catches a call | ||
| //! written from upstream's order — read the signature, not the memory of the | ||
| //! Python one. | ||
| //! | ||
| //! The algorithm is identical for every backend; only the three primitive | ||
| //! emitters it drives are not, which is why they are the trait and this is a | ||
| //! free function over it. | ||
|
|
||
| use crate::arch::WORD; | ||
| use crate::regloc::Loc; | ||
| use indexmap::IndexMap; | ||
|
|
||
| /// The three emitters `remap_frame_layout` drives, named as `jump.py` calls | ||
| /// them on the `assembler` it is handed. | ||
| /// | ||
| /// `regalloc_push`/`regalloc_pop` exist only to serve the cycle-breaking arm | ||
| /// below — parking one value of a cycle on the machine stack is the whole | ||
| /// reason a cycle can be resolved at all. | ||
| pub(crate) trait RegallocMoves { | ||
| /// `assembler.py:1145 regalloc_mov(from_loc, to_loc)`. | ||
| fn regalloc_mov(&mut self, src: &Loc, dst: &Loc); | ||
| fn regalloc_push(&mut self, loc: &Loc); | ||
| fn regalloc_pop(&mut self, loc: &Loc); | ||
| } | ||
|
|
||
| /// A location's identity for the dependency bookkeeping. | ||
| /// | ||
| /// Registers and frame slots share one number space, so the constants keep the | ||
| /// classes apart; an immediate has no identity because it is never anyone's | ||
| /// destination and can be re-materialised at will. | ||
| pub(crate) fn loc_as_key(loc: &Loc) -> i32 { | ||
| match loc { | ||
| Loc::Reg(r) if r.is_xmm => 0x2000 + i32::from(r.value), | ||
| Loc::Reg(r) => 0x1000 + i32::from(r.value), | ||
| Loc::Frame(f) => f.ebp_loc.value, | ||
| Loc::Ebp(e) => e.value, | ||
| Loc::Immed(_) => i32::MIN, | ||
| Loc::Addr(a) => a.offset, | ||
| } | ||
| } | ||
|
Comment on lines
+43
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Determine the ebp/frame offset ranges reachable per backend and
# compare them against the 0x1000 / 0x2000 register key bases in loc_as_key.
set -euo pipefail
# The offset producer used to build Loc::Frame / Loc::Ebp.
ast-grep run --pattern 'pub fn get_ebp_ofs($$$) { $$$ }' --lang rust majit/majit-backend-dynasm/src || true
rg -nP -C6 '\bfn get_ebp_ofs\s*\(' --type=rust majit/majit-backend-dynasm
# FrameLoc construction sites: is `ebp_loc.value` a positive byte offset?
rg -nP -C4 'FrameLoc::new\s*\(' --type=rust majit/majit-backend-dynasm
# FIRST_ITEM_OFFSET and WORD: the aarch64 slot->offset scale.
rg -nP -C3 '\b(FIRST_ITEM_OFFSET|const WORD)\b' --type=rust majit/majit-backend-dynasm
# Any existing bound on frame_depth / slot count that would keep offsets < 4096.
rg -nP -C3 '\b(frame_depth|next_slot)\s*[:=]' --type=rust majit/majit-backend-dynasm/src/jitframe.rs majit/majit-backend-dynasm/src/regalloc.rs 2>/dev/null || trueRepository: youknowone/pyre Length of output: 33414 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository ---'
git rev-parse --show-toplevel
printf '%s\n' '--- jump outline ---'
ast-grep outline majit/majit-backend-dynasm/src/jump.rs
printf '%s\n' '--- jump implementation and tests ---'
sed -n '1,240p' majit/majit-backend-dynasm/src/jump.rs
rg -n -C4 'loc_as_key|srccount|pending_dests|remap_frame_layout|remap_frame_layout_mixed|WORD' \
majit/majit-backend-dynasm/src/jump.rs
printf '%s\n' '--- frame constants and depth allocation ---'
rg -n -C5 'FIRST_ITEM_OFFSET|JITFRAME_FIXED_SIZE|SIZEOFSIGNED|get_frame_depth|frame_depth|frame_size|next_slot' \
majit/majit-backend-dynasm/src/jitframe.rs \
majit/majit-backend-dynasm/src/regalloc.rs \
majit/majit-backend-dynasm/src/x86 \
majit/majit-backend-dynasm/src/aarch64
printf '%s\n' '--- local upstream/source references ---'
rg -n -C4 '_getregkey|getregkey|remap_frame_layout|pending_dests|srccount' . \
-g '*.py' -g '*.rs' -g '*.txt' | head -300
printf '%s\n' '--- deterministic collision model ---'
python3 - <<'PY'
REG_GPR = lambda v: 0x1000 + v
REG_XMM = lambda v: 0x2000 + v
for base in (0, 0x100, 0x1000, 0x2000):
print("base", base, "gpr collision offset", base + 8 * (0x1000 - base // 8))
print("base", base, "xmm collision offset", base + 8 * (0x2000 - base // 8))
print("direct examples:", REG_GPR(0) == 0x1000, REG_XMM(0) == 0x2000)
PYRepository: youknowone/pyre Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate frame constants ---'
fd -t f -e rs | while read -r f; do
rg -n -C3 'pub const (FIRST_ITEM_OFFSET|SIZEOFSIGNED|JITFRAME_FIXED_SIZE)|FIRST_ITEM_OFFSET' "$f" && true
done | head -240
printf '%s\n' '--- location definitions ---'
fd -t f -e rs majit | while read -r f; do
if rg -q 'enum Loc|struct FrameLoc|struct EbpLoc|struct AddrLoc|enum.*Loc' "$f"; then
echo "FILE: $f"
ast-grep outline "$f" | rg 'Loc|FrameLoc|Ebp|Addr'
rg -n -C5 'enum Loc|struct FrameLoc|struct EbpLoc|struct AddrLoc|Loc::Ebp|Loc::Addr|fn is_stack|fn is_immed' "$f"
fi
done | head -360
printf '%s\n' '--- jump call sites and location construction ---'
rg -n -C5 'remap_frame_layout(_mixed)?\s*\(|Loc::Ebp|Loc::Addr|EbpLoc|AddrLoc' \
majit/majit-backend-dynasm/src \
-g '*.rs' | head -360
printf '%s\n' '--- minimal loop model ---'
python3 - <<'PY'
# Model the relevant bookkeeping for two distinct locations that share a key.
# Both sources are immediates, so no source decrements srccount.
dst_keys = [0x1000, 0x1000]
pending = len(dst_keys)
srccount = {}
for key in dst_keys:
srccount[key] = 0
steps = []
while pending > 0 and len(steps) < 4:
progress = False
for key in dst_keys:
if srccount.get(key, -1) == 0:
srccount[key] = -1
pending -= 1
progress = True
steps.append(("move", hex(key), pending))
if not progress:
steps.append(("no_progress", pending))
break
print("steps:", steps)
print("loop_condition_remains_true:", pending > 0)
PYRepository: youknowone/pyre Length of output: 10817 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate frame constants ---'
fd -t f -e rs | while read -r f; do
rg -n -C3 'pub const (FIRST_ITEM_OFFSET|SIZEOFSIGNED|JITFRAME_FIXED_SIZE)|FIRST_ITEM_OFFSET' "$f" && true
done | head -240
printf '%s\n' '--- location definitions ---'
fd -t f -e rs majit | while read -r f; do
if rg -q 'enum Loc|struct FrameLoc|struct EbpLoc|struct AddrLoc|enum.*Loc' "$f"; then
echo "FILE: $f"
ast-grep outline "$f" | rg 'Loc|FrameLoc|Ebp|Addr'
rg -n -C5 'enum Loc|struct FrameLoc|struct EbpLoc|struct AddrLoc|Loc::Ebp|Loc::Addr|fn is_stack|fn is_immed' "$f"
fi
done | head -360
printf '%s\n' '--- jump call sites and location construction ---'
rg -n -C5 'remap_frame_layout(_mixed)?\s*\(|Loc::Ebp|Loc::Addr|EbpLoc|AddrLoc' \
majit/majit-backend-dynasm/src \
-g '*.rs' | head -360
printf '%s\n' '--- minimal loop model ---'
python3 - <<'PY'
dst_keys = [0x1000, 0x1000]
pending = len(dst_keys)
srccount = {}
for key in dst_keys:
srccount[key] = 0
steps = []
while pending > 0 and len(steps) < 4:
progress = False
for key in dst_keys:
if srccount.get(key, -1) == 0:
srccount[key] = -1
pending -= 1
progress = True
steps.append(("move", hex(key), pending))
if not progress:
steps.append(("no_progress", pending))
break
print("steps:", steps)
print("loop_condition_remains_true:", pending > 0)
PYRepository: youknowone/pyre Length of output: 10817 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- exact shared jitframe constants ---'
fd -t f -e rs | while read -r f; do
if rg -q 'pub const (BASEITEMOFS|JF_FRAME_OFS|FIRST_ITEM_OFFSET)|struct JitFrame' "$f"; then
echo "FILE: $f"
sed -n '1,190p' "$f"
fi
done
printf '%s\n' '--- exact location types ---'
fd -t f -e rs | while read -r f; do
if rg -q 'pub enum Loc|enum Loc|pub struct FrameLoc|struct FrameLoc|pub struct EbpLoc|struct EbpLoc' "$f"; then
echo "FILE: $f"
rg -n -C8 'pub enum Loc|enum Loc|pub struct FrameLoc|struct FrameLoc|pub struct EbpLoc|struct EbpLoc|pub struct AddrLoc|struct AddrLoc|fn is_stack|fn is_immed' "$f" || true
fi
done
printf '%s\n' '--- location construction in jump callers ---'
rg -n -C3 'Loc::Ebp|Loc::Addr|EbpLoc|AddrLoc|remap_frame_layout(_mixed)?\s*\(' \
majit/majit-backend-dynasm/src -g '*.rs' || true
printf '%s\n' '--- frame-depth bounds ---'
rg -n -C4 'current_frame_depth|_increase_frame_depth|frame_depth.*assert|frame_depth.*max|MAX.*FRAME|FRAME.*MAX' \
majit/majit-backend-dynasm/src -g '*.rs' || trueRepository: youknowone/pyre Length of output: 50373 🌐 Web query:
💡 Result: In the RPython JIT backend (specifically for architectures like ARM), remap_frame_layout is a function used during register allocation and code generation to manage the movement of data between locations when preparing for jumps or bridge transitions [1][2][3]. It is typically found within the jump.py module, such as rpython/jit/backend/arm/jump.py [1][2]. The StackLoc._getregkey method—often implemented via an as_key method in the location hierarchy—is used by the register allocator to uniquely identify stack locations for mapping and remapping purposes [4]. The exact key formula for stack locations is defined as follows: For a generic stack location (StackLocation), the key formula is: as_key = self.position + 10000 [4] For a raw stack pointer location (RawSPStackLocation), which represents a word in the stack frame relative to the stack pointer, the key formula is: as_key = self.value + 1000 [4] These keys allow the JIT's register allocator to distinguish between different types of locations (such as core registers, floating-point registers, immediate values, and various stack-based positions) by assigning them unique, non-overlapping integer ranges [4]. Citations:
🌐 Web query:
💡 Result: In the PyPy/RPython source code, the Citations:
🏁 Script executed: #!/bin/bash
set -uo pipefail
printf '%s\n' '--- exact location definitions ---'
sed -n '1,230p' majit/majit-backend-dynasm/src/regloc.rs
printf '%s\n' '--- remap call sites only ---'
rg -n -C6 'remap_frame_layout(_mixed)?\s*\(' \
majit/majit-backend-dynasm/src/x86 \
majit/majit-backend-dynasm/src/aarch64 \
majit/majit-backend-dynasm/src/regalloc.rs || true
printf '%s\n' '--- non-frame location construction only ---'
rg -n -C3 'Loc::(Ebp|Addr)|RawEbpLoc|AddressLoc|AddrLoc' \
majit/majit-backend-dynasm/src/x86 \
majit/majit-backend-dynasm/src/aarch64 \
majit/majit-backend-dynasm/src/regalloc.rs \
majit/majit-backend-dynasm/src/regloc.rs || trueRepository: youknowone/pyre Length of output: 27137 🏁 Script executed: #!/bin/bash
set -uo pipefail
for branch in default main; do
for file in rpython/jit/backend/x86/locations.py rpython/jit/backend/x86/jump.py; do
url="https://raw.githubusercontent.com/pypy/pypy/${branch}/${file}"
echo "--- $url ---"
curl -fsSL --max-time 15 "$url" 2>/dev/null \
| rg -n -C8 '_getregkey|class StackLoc|def remap_frame_layout|srccount|pending_dests|class RawStackLoc|class AddressLoc' \
|| echo "unavailable"
done
doneRepository: youknowone/pyre Length of output: 5515 🏁 Script executed: #!/bin/bash
set -uo pipefail
for file in rpython/jit/backend/x86/regloc.py rpython/jit/backend/arm/regloc.py rpython/jit/backend/aarch64/regloc.py; do
url="https://raw.githubusercontent.com/pypy/pypy/main/$file"
echo "--- $url ---"
curl -fsSL --max-time 15 "$url" 2>/dev/null \
| rg -n -C10 '_getregkey|class .*Loc|class StackLoc|class FrameLoc|class Raw' \
|| echo "unavailable"
doneRepository: youknowone/pyre Length of output: 7352 Use collision-free location keys.
Use a tagged key for every location class. Update the wide-stack check in 🤖 Prompt for AI Agents |
||
|
|
||
| pub(crate) fn loc_width(loc: &Loc) -> usize { | ||
| match loc { | ||
| Loc::Reg(r) => r.get_width(), | ||
| Loc::Frame(f) => f.ebp_loc.get_width(), | ||
| Loc::Ebp(e) => e.get_width(), | ||
| _ => WORD, | ||
| } | ||
| } | ||
|
|
||
| /// `jump.py:4 remap_frame_layout` — emit the moves that put `src_locations` | ||
| /// into `dst_locations`, in an order no move invalidates. | ||
| /// | ||
| /// `tmpreg` is needed for a stack-to-stack pair, which no machine here can | ||
| /// move in one instruction. | ||
| pub(crate) fn remap_frame_layout<A: RegallocMoves + ?Sized>( | ||
| asm: &mut A, | ||
| src_locations: &[Loc], | ||
| dst_locations: &[Loc], | ||
| tmpreg: Loc, | ||
| ) { | ||
| let mut pending_dests = dst_locations.len() as i32; | ||
| let mut srccount: IndexMap<i32, i32> = IndexMap::new(); | ||
| for dst in dst_locations { | ||
| srccount.insert(loc_as_key(dst), 0); | ||
| } | ||
| for i in 0..dst_locations.len() { | ||
| let src = src_locations[i]; | ||
| if src.is_immed() { | ||
| continue; | ||
| } | ||
| let key = loc_as_key(&src); | ||
| if let Some(cnt) = srccount.get_mut(&key) { | ||
| if key == loc_as_key(&dst_locations[i]) { | ||
| *cnt = -(dst_locations.len() as i32) - 1; | ||
| pending_dests -= 1; | ||
| } else { | ||
| *cnt += 1; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| while pending_dests > 0 { | ||
| let mut progress = false; | ||
| for i in 0..dst_locations.len() { | ||
| let dst = dst_locations[i]; | ||
| let key = loc_as_key(&dst); | ||
| if srccount.get(&key).copied().unwrap_or(-1) == 0 { | ||
| srccount.insert(key, -1); | ||
| pending_dests -= 1; | ||
| let src = src_locations[i]; | ||
| if !src.is_immed() { | ||
| let src_key = loc_as_key(&src); | ||
| if let Some(cnt) = srccount.get_mut(&src_key) { | ||
| *cnt -= 1; | ||
| } | ||
| } | ||
| if dst.is_stack() && src.is_stack() { | ||
| asm.regalloc_mov(&src, &tmpreg); | ||
| asm.regalloc_mov(&tmpreg, &dst); | ||
| } else { | ||
| asm.regalloc_mov(&src, &dst); | ||
| } | ||
| progress = true; | ||
| } | ||
| } | ||
| if !progress { | ||
| let mut sources: IndexMap<i32, Loc> = IndexMap::new(); | ||
| for i in 0..dst_locations.len() { | ||
| sources.insert(loc_as_key(&dst_locations[i]), src_locations[i]); | ||
| } | ||
| for dst in dst_locations { | ||
| let originalkey = loc_as_key(dst); | ||
| if srccount.get(&originalkey).copied().unwrap_or(-1) >= 0 { | ||
| asm.regalloc_push(dst); | ||
| let mut cur_dst = *dst; | ||
| loop { | ||
| let key = loc_as_key(&cur_dst); | ||
| srccount.insert(key, -1); | ||
| pending_dests -= 1; | ||
| let src = sources[&key]; | ||
| if loc_as_key(&src) == originalkey { | ||
| break; | ||
| } | ||
| if cur_dst.is_stack() && src.is_stack() { | ||
| asm.regalloc_mov(&src, &tmpreg); | ||
| asm.regalloc_mov(&tmpreg, &cur_dst); | ||
| } else { | ||
| asm.regalloc_mov(&src, &cur_dst); | ||
| } | ||
| cur_dst = src; | ||
| } | ||
| asm.regalloc_pop(&cur_dst); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// jump.py:4 remap_frame_layout — emit moves to rearrange locations. | ||
| /// `jump.py:67 remap_frame_layout_mixed` — two location sets remapped with a | ||
| /// temporary each, as integer and float arguments need. | ||
| /// | ||
| /// This handles the problem of parallel assignment: if src[i] overlaps | ||
| /// with dst[j], we need a temporary to break the cycle. | ||
| pub fn remap_frame_layout(_src_locs: &[Loc], _dst_locs: &[Loc], _tmpreg: Loc) -> Vec<(Loc, Loc)> { | ||
| // TODO: implement cycle-breaking parallel move algorithm | ||
| // For now, return direct moves (incorrect if overlaps exist) | ||
| Vec::new() | ||
| /// The sets are not independent: a set-2 stack source may be a set-1 | ||
| /// destination, and set 1 runs first. Those sources are pushed before either | ||
| /// remap and popped into place after, which is why they are dropped from the | ||
| /// set-2 lists rather than reordered. | ||
| pub(crate) fn remap_frame_layout_mixed<A: RegallocMoves + ?Sized>( | ||
| asm: &mut A, | ||
| src_locations1: &[Loc], | ||
| dst_locations1: &[Loc], | ||
| tmpreg1: Loc, | ||
| src_locations2: &[Loc], | ||
| dst_locations2: &[Loc], | ||
| tmpreg2: Loc, | ||
| ) { | ||
| let mut extrapushes = Vec::new(); | ||
| let mut dst_keys = IndexMap::new(); | ||
| for loc in dst_locations1 { | ||
| dst_keys.insert(loc_as_key(loc), ()); | ||
| } | ||
| let mut src_locations2red = Vec::new(); | ||
| let mut dst_locations2red = Vec::new(); | ||
| for i in 0..src_locations2.len() { | ||
| let loc = src_locations2[i]; | ||
| let dstloc = dst_locations2[i]; | ||
| if loc.is_stack() { | ||
| let key = loc_as_key(&loc); | ||
| if dst_keys.contains_key(&key) | ||
| || (loc_width(&loc) > WORD && dst_keys.contains_key(&(key + WORD as i32))) | ||
| { | ||
| asm.regalloc_push(&loc); | ||
| extrapushes.push(dstloc); | ||
| continue; | ||
| } | ||
| } | ||
| src_locations2red.push(loc); | ||
| dst_locations2red.push(dstloc); | ||
| } | ||
| remap_frame_layout(asm, src_locations1, dst_locations1, tmpreg1); | ||
| remap_frame_layout(asm, &src_locations2red, &dst_locations2red, tmpreg2); | ||
| while let Some(loc) = extrapushes.pop() { | ||
| asm.regalloc_pop(&loc); | ||
| } | ||
| } | ||
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
RegallocMovesstates no operand contract, so both backends silently drop moves. The trait accepts any&Loc, andloc_as_keyassigns dependency keys toLoc::EbpandLoc::Addras well as toReg,Frame, andImmed.remap_frame_layouttherefore schedules moves forEbpandAddrlocations and counts them inpending_dests, while each implementation ends in a_ => {}arm that emits nothing. The destination keeps a stale value, and a droppedregalloc_popadditionally leavesrspshifted.majit/majit-backend-dynasm/src/jump.rs#L31-L36: document the operand kinds each emitter must accept, and restrictloc_as_keyto those kinds so the algorithm cannot schedule a move no backend can emit.majit/majit-backend-dynasm/src/aarch64/assembler.rs#L7984-L7984: replace the_ => {}arm inregalloc_movwith a panic that reports the operand pair, and do the same for theregalloc_pushandregalloc_popwildcards at Lines 8004 and 8025.majit/majit-backend-dynasm/src/x86/assembler.rs#L8875-L8875: replace the_ => {}arm inregalloc_movwith the same panic, and do the same for theregalloc_pushandregalloc_popwildcards at Lines 8893 and 8911.📍 Affects 3 files
majit/majit-backend-dynasm/src/jump.rs#L31-L36(this comment)majit/majit-backend-dynasm/src/aarch64/assembler.rs#L7984-L7984majit/majit-backend-dynasm/src/x86/assembler.rs#L8875-L8875🤖 Prompt for AI Agents