Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions majit/majit-metainterp/src/jitdriver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,31 @@ pub fn no_bridge_enabled() -> bool {
static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
*FLAG.get_or_init(|| std::env::var_os("MAJIT_NO_BRIDGE").is_some())
}
/// `MAJIT_MAX_BRIDGES=N` (diagnostic): allow the first N bridge compilations
/// and behave as `MAJIT_NO_BRIDGE` from then on. Bisecting N names the bridge
/// whose compilation first produces a wrong value, at seconds per run rather
/// than a rebuild per arm. Consumes fuel only when the rest of `should_bridge`
/// already held, so the count is bridges actually taken — place it last in the
/// `&&` chain. `MAJIT_BRIDGE_FUEL_LOG` reports each one taken.
fn bridge_fuel_take() -> bool {
Comment on lines +469 to +475

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate CALL_ASSEMBLER bridges with the fuel counter

When a guard failure arrives through CALL_ASSEMBLER, MAJIT_MAX_BRIDGES does not limit it: jit_ca_handle_guard_failure and try_compile_ca_bridge perform their own must_compile_with_values checks and call trace_and_compile_from_bridge directly (pyre/pyre-jit/src/call_jit.rs:3959-4017 and 4074-4109), while that function checks only no_bridge_enabled at lines 3240-3244. Consequently even MAJIT_MAX_BRIDGES=0 can compile these bridges, and larger limits no longer identify the global Nth bridge as documented. Make the fuel gate available to these frontend-owned bridge decisions, just as MAJIT_NO_BRIDGE is.

AGENTS.md reference: AGENTS.md:L252-L253

Useful? React with 👍 / 👎.

static LIMIT: std::sync::OnceLock<Option<u64>> = std::sync::OnceLock::new();
let Some(limit) = *LIMIT.get_or_init(|| {
std::env::var("MAJIT_MAX_BRIDGES")
.ok()
.and_then(|v| v.parse().ok())
}) else {
return true;
};
static USED: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
let n = USED.fetch_add(1, std::sync::atomic::Ordering::Relaxed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Spend bridge fuel only for an actual bridge attempt

With MAJIT_MAX_BRIDGES set, this counter advances before bridge tracing is known to be possible. In the green-resume path, for example, should_bridge consumes fuel before blackhole recovery can return pc == usize::MAX, which skips start_bridge_tracing; other paths call start_bridge_tracing, which can normally return false for !state.can_trace(), an evicted owning token, or a declined retrace. Thus MAJIT_MAX_BRIDGES=1 can consume its only slot without compiling any bridge, contradicting the documented “first N bridge compilations” semantics and misleading the intended bisection. Count or log only after the final bridge-path gates have succeeded.

AGENTS.md reference: AGENTS.md:L252-L253

Useful? React with 👍 / 👎.

if n >= limit {
return false;
}
if std::env::var_os("MAJIT_BRIDGE_FUEL_LOG").is_some() {
eprintln!("@@@FUEL bridge #{n}");
}
true
}
fn guardlog_enabled() -> bool {
static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
*FLAG.get_or_init(|| std::env::var_os("MAJIT_GUARDLOG").is_some())
Expand Down Expand Up @@ -4413,7 +4438,8 @@ impl<S: JitState> JitDriver<S> {
// pending-field prologue (resume.py:993-1007).
let should_bridge = must_compile
&& !majit_metainterp::MetaInterp::<S::Meta>::stack_almost_full()
&& !no_bridge_enabled();
&& !no_bridge_enabled()
&& bridge_fuel_take();

// compile.py:710 recovery_layout header_pc parity:
// guard resume_pc comes from the guard's recovery metadata.
Expand Down Expand Up @@ -5715,7 +5741,8 @@ impl<S: JitState> JitDriver<S> {
// every bridge.
let should_bridge = must_compile
&& !majit_metainterp::MetaInterp::<S::Meta>::stack_almost_full()
&& !no_bridge_enabled();
&& !no_bridge_enabled()
&& bridge_fuel_take();

// Same `@@@GUARD` line the sibling loops emit. Without it this loop —
// the one pyre reaches — had no per-guard-failure trace at all, so
Expand Down Expand Up @@ -6860,7 +6887,8 @@ impl<S: JitState> JitDriver<S> {
// resume defects from the blackhole path.
let should_bridge = must_compile
&& !majit_metainterp::MetaInterp::<S::Meta>::stack_almost_full()
&& !no_bridge_enabled();
&& !no_bridge_enabled()
&& bridge_fuel_take();

// compile.py:710 recovery_layout header_pc parity:
// guard resume_pc comes from the guard's recovery metadata.
Expand Down
77 changes: 74 additions & 3 deletions majit/majit-translate/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4053,13 +4053,30 @@ pub fn prune_dead_phis(graph: &mut FunctionGraph) {
// Step 6: trim Link.args at indices whose target inputarg is dead.
// `simplify.py:512-516`. Walk in reverse so removals don't shift
// surviving indices.
//
// The scope is the *target*'s reachability, not the source's. Step 7
// trims the inputargs of every reachable block, and a block the
// reachability walk excludes — an orphan `eliminate_empty_blocks`
// bypassed, or one of the merge blocks jtransform leaves whose
// inputargs are phi targets rather than parameters — can still name a
// reachable block as its link target. Skipping that link because its
// own block is unreachable breaks `len(link.args) ==
// len(link.target.inputargs)` with no diagnostic:
// `remove_duplicate_inputargs` reads each column by index across every
// incoming link, so the untrimmed one contributes the value one slot
// over, and the union-find merges two variables that are not the same
// value — a rename applied to the whole graph. Reachability is closed
// under exits, so this only ever adds links, never drops one.
//
// Upstream cannot reach this: `graph.iterblocks()` *is* its block list,
// so an unreachable block is not in `blocks` and has no link to skip.
for block_idx in 0..graph.blocks.len() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Exclude unreachable links instead of rewriting them

When FunctionGraph retains the orphan predecessor described here, extending Step 6 to every source block is a workaround in the wrong pass: upstream transform_dead_op_vars walks only its reachable blocks, while upstream mkentrymap(graph) also sees only reachable iterlinks. The later Rust remove_duplicate_inputargs still builds its entry map from every raw graph.blocks link, so the orphan continues to participate in phi-tuple equality and can suppress legitimate duplicate-phi elimination. Preserve the upstream structure by filtering unreachable links when constructing that entry map (or removing the orphan), rather than making dead-variable trimming process links upstream never sees.

AGENTS.md reference: AGENTS.md:L231-L234

Useful? React with 👍 / 👎.

if !reachable.contains(&graph.blocks[block_idx].id) {
continue;
}
let exits_len = graph.blocks[block_idx].exits.len();
for exit_idx in 0..exits_len {
let target = graph.blocks[block_idx].exits[exit_idx].target;
if !reachable.contains(&target) {
continue;
}
let target_iargs: Vec<crate::flowspace::model::Variable> = {
let &i = block_index
.get(&target)
Expand Down Expand Up @@ -6312,6 +6329,60 @@ mod tests {
assert!(!has_phi_op, "orphan phi `OpKind::Input` must be dropped");
}

#[test]
fn prune_dead_phis_trims_an_unreachable_predecessors_link_with_the_target() {
// entry ─┐
// ├→ merge(inputargs [dead, live]) → returnblock(live)
// orphan ─┘
//
// `orphan` has no predecessor and is not a calling-convention entry —
// its inputarg is a phi target with no backing `OpKind::Input`, the
// shape `jtransform` leaves behind — so the reachability walk excludes
// it. Steps 6 and 7 both walk `reachable`, so `merge`'s dead inputarg
// goes while `orphan`'s link keeps both args, and every later consumer
// that zips a link against its target's inputargs is then off by one.
let mut graph = FunctionGraph::new("test");
let entry = graph.startblock;
let e_dead = graph.push_op_var(entry, OpKind::ConstInt(1), true).unwrap();
let e_live = graph.push_op_var(entry, OpKind::ConstInt(2), true).unwrap();

let merge = graph.create_block();
install_phi(&mut graph, merge, "dead");
let live_phi = install_phi(&mut graph, merge, "live");
graph.set_goto(entry, merge, vec![e_dead, e_live.clone()]);
graph.set_return(merge, Some(live_phi));

let orphan = graph.create_block();
let stranded = graph.alloc_value_var();
graph.push_inputarg_var(orphan, stranded);
let o_dead = graph
.push_op_var(orphan, OpKind::ConstInt(3), true)
.unwrap();
let o_live = graph
.push_op_var(orphan, OpKind::ConstInt(4), true)
.unwrap();
graph.set_goto(orphan, merge, vec![o_dead, o_live.clone()]);

prune_dead_phis(&mut graph);

assert_eq!(
graph.block(merge).inputargs.len(),
1,
"the unread phi column is dropped"
);
assert_eq!(
graph.block(entry).exits[0].args,
vec![LinkArg::Value(e_live)],
"the reachable predecessor keeps the live column"
);
assert_eq!(
graph.block(orphan).exits[0].args,
vec![LinkArg::Value(o_live)],
"an unreachable predecessor's link must be trimmed with its target, \
not left naming the column that was dropped"
);
}

#[test]
fn prune_dead_phis_retains_live_single_source_phi_pending_ssa_to_ssi() {
// entry -> merge(phi 'x' read by a BinOp whose result is the
Expand Down
13 changes: 12 additions & 1 deletion pyre/pyre-interpreter/src/pyframe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,21 @@ macro_rules! locals_w {
}

/// Mutably borrow a frame's `locals_cells_stack_w` array. See [`locals_w!`].
///
/// The `&mut *` on the receiver is load-bearing: the field it projects is a
/// raw pointer, which reads fine through a shared `&PyFrame`, so without it a
/// safe caller holding a shared frame could mint `&mut` to the array. The
/// accessor this replaced took `&mut self` and this restores that requirement.
///
/// It does not bound the result's lifetime — `&mut *ptr` has an unconstrained
/// one — so two overlapping calls still produce aliasing the borrow checker
/// cannot see. Tying the lifetime needs a function signature to tie it to,
/// which is the accessor form that puts the `getfield` in its own graph and
/// defeats the pairing described on [`locals_w!`].
#[macro_export]
macro_rules! locals_w_mut {
($frame:expr) => {
(unsafe { &mut *$frame.locals_cells_stack_w })
(unsafe { &mut *(&mut *$frame).locals_cells_stack_w })
};
}

Expand Down
Loading