unroll: guard short-preamble force-loop on unmapped jump arg#556
Merged
Conversation
WalkthroughShort-preamble optimization now hoists postponed overflow-checked operations and tightens replay fixpoint handling so unmapped non-constant jump arguments are deferred until their producers are registered. ChangesShort-preamble optimization fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 367d038). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
inline_short_preamble's inner force-box loop re-forces short_jump_args
until the list stops growing. When force_box grows it via add_preamble_op
with a producer short-op not yet replayed, the newly-added jump arg is
absent from `mapping`; the re-loop's top-of-loop _map_args then panicked
on `expect("mapping missing jump_arg")`, aborting the process.
Port the unroll.py:425-434 guard: after the list grows, break out of the
inner loop when any new non-Const entry is not yet in `mapping`, deferring
to the outer replay loop to register it, and only re-force when every new
entry is a Const or already mapped.
Assisted-by: Claude
OptPure records is_always_pure ops into short_preamble_pure_ops so produce_potential_short_preamble_ops can replay them into the peeled short preamble. The OVF-op emit branch (an is_ovf op followed by GUARD_NO_OVERFLOW) only inserted into the CSE cache and never pushed to short_preamble_pure_ops, so a loop-invariant overflow-checked op (INT_ADD_OVF/INT_SUB_OVF/INT_MUL_OVF) was recomputed each iteration instead of hoisted once into the preamble. Push the emitted OVF op onto short_preamble_pure_ops, matching pure.py:321-322 which calls sb.add_pure_op(op) for both is_always_pure and is_ovf+GUARD_NO_OVERFLOW ops. Assisted-by: Claude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-metainterp/src/optimizeopt/unroll.rs`:
- Around line 3980-3993: Update the length guard in the short-jump argument
handling around current_short_jump_args so it exits when grown.len() is less
than or equal to num_short_jump_args, before slicing
grown[num_short_jump_args..]. Preserve the existing growth and remapping
behavior for longer argument lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c720a139-7ce7-4bd7-8fd0-813fc94b8e9a
📒 Files selected for processing (2)
majit/majit-metainterp/src/optimizeopt/pure.rsmajit/majit-metainterp/src/optimizeopt/unroll.rs
Comment on lines
+3980
to
+3993
| // unroll.py:425-434: if short_jump_args did not grow we are | ||
| // done. If force_box grew it via add_preamble_op, only re-force | ||
| // when every new entry is a Const or already in `mapping`; | ||
| // otherwise break and let the outer replay loop register the | ||
| // producer first, since the top-of-loop _map_args above would | ||
| // otherwise read an unmapped arg. | ||
| let grown = current_short_jump_args(short_preamble, ctx); | ||
| if grown.len() == num_short_jump_args { | ||
| break; | ||
| } | ||
| let all_new_mapped = grown[num_short_jump_args..] | ||
| .iter() | ||
| .all(|arg| arg.is_constant() || mapping.contains_key(arg)); | ||
| if !all_new_mapped { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use a defensive length check to prevent potential slice panics.
While short_jump_args logically only grows in this context, using <= instead of == is a good defensive practice. It prevents a potential out-of-bounds slice panic at grown[num_short_jump_args..] if the underlying builder logic is ever refactored in the future to allow shrinking.
🛠️ Proposed defensive fix
- if grown.len() == num_short_jump_args {
+ if grown.len() <= num_short_jump_args {
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // unroll.py:425-434: if short_jump_args did not grow we are | |
| // done. If force_box grew it via add_preamble_op, only re-force | |
| // when every new entry is a Const or already in `mapping`; | |
| // otherwise break and let the outer replay loop register the | |
| // producer first, since the top-of-loop _map_args above would | |
| // otherwise read an unmapped arg. | |
| let grown = current_short_jump_args(short_preamble, ctx); | |
| if grown.len() == num_short_jump_args { | |
| break; | |
| } | |
| let all_new_mapped = grown[num_short_jump_args..] | |
| .iter() | |
| .all(|arg| arg.is_constant() || mapping.contains_key(arg)); | |
| if !all_new_mapped { | |
| // unroll.py:425-434: if short_jump_args did not grow we are | |
| // done. If force_box grew it via add_preamble_op, only re-force | |
| // when every new entry is a Const or already in `mapping`; | |
| // otherwise break and let the outer replay loop register the | |
| // producer first, since the top-of-loop _map_args above would | |
| // otherwise read an unmapped arg. | |
| let grown = current_short_jump_args(short_preamble, ctx); | |
| if grown.len() <= num_short_jump_args { | |
| break; | |
| } | |
| let all_new_mapped = grown[num_short_jump_args..] | |
| .iter() | |
| .all(|arg| arg.is_constant() || mapping.contains_key(arg)); | |
| if !all_new_mapped { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/optimizeopt/unroll.rs` around lines 3980 - 3993,
Update the length guard in the short-jump argument handling around
current_short_jump_args so it exits when grown.len() is less than or equal to
num_short_jump_args, before slicing grown[num_short_jump_args..]. Preserve the
existing growth and remapping behavior for longer argument lists.
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
1 participant
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.
One commit porting the upstream
inline_short_preambleinner fix-point guard.The bug
inline_short_preamble's inner force-box loop re-forcesshort_jump_argsuntil the list stops growing. Whenforce_boxgrows the list viaadd_preamble_opwith a producer short-op not yet replayed, the newly-added jump arg is absent frommapping; on the immediate re-loop the top-of-loop_map_argspanicked (expect("mapping missing jump_arg")), aborting the JIT process instead of deferring.The fix
Port
unroll.py:425-434(pypy fix983edc22e589/ gh-5212): after the list grows, break out of the inner loop when any new non-Constentry is not yet inmapping, deferring to the outer replay loop to register it; only re-force when every new entry is aConstor already mapped. The stale comment citing the pre-guard range (417-423) is updated to417-434.Verification
majit-metainterplib: dynasm 1396 / cranelift 1394, 0 failedcheck.py: dynasm 181/181, cranelift 181/181PYRE_JIT=0oracle == JIT on both backends— commented by Claude
Summary by CodeRabbit
Bug Fixes
Performance