Skip to content

majit: Defer Dynasm bridge constant conversion to diagnostic path - #1299

Merged
youknowone merged 1 commit into
youknowone:mainfrom
notJoon:refactor/lazy-dynasm-bridge-constants
Aug 17, 2026
Merged

majit: Defer Dynasm bridge constant conversion to diagnostic path#1299
youknowone merged 1 commit into
youknowone:mainfrom
notJoon:refactor/lazy-dynasm-bridge-constants

Conversation

@notJoon

@notJoon notJoon commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Move the raw constant map conversion into the PYRE_TRACE_OPS_DIAG branch, avoiding diagnostic-only allocation and iteration during normal bridge compilation.

compile_loop has similar const_pool collection logic, but I left it duplicated rather than extracting a helper that isn't needed yet.

let constants: indexmap::IndexMap<u32, i64> = const_pool
.iter()
.map(|(&key, value)| (key, value.as_raw_i64()))
.collect();

Self-review

  • I fully resolved all reasonable code review comments from Codex and CodeRabbit.
    • Auto-review section 1 is clear. This check is mandatory.
    • Auto-review section 2 is clear. If this is not checked, please add a comment explaining why.
  • I did not use AI to write the code of this patch.
    • If this is not checked, commits must include Assisted-by

Summary by CodeRabbit

  • Refactor
    • Streamlined internal diagnostic handling without changing the resulting output or end-user behavior.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 60e713c0-5a32-4701-85ba-86979673c057

📥 Commits

Reviewing files that changed from the base of the PR and between 21ce530 and 4ba378b.

📒 Files selected for processing (1)
  • majit/majit-backend-dynasm/src/runner.rs

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.


Walkthrough

The change reorders trace-diagnostic evaluation in compile_bridge and uses the imported IndexMap type for the raw constant map.

Changes

Compile bridge diagnostic ordering

Layer / File(s) Summary
Trace diagnostic setup
majit/majit-backend-dynasm/src/runner.rs
compile_bridge checks the trace-diagnostic environment before constructing the raw constant map. The map now uses the imported IndexMap type.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: 🟡 Moderate · up to 4ba37

The Dynasm backend change is not merge-ready until the required checks run successfully or their omission is explicitly accepted; the current environment lacks the Rust toolchain needed to execute them.

Possibly related PRs

Suggested reviewers: lifthrasiir, youknowone

Poem

A rabbit checks the trace with care,
Then maps the constants in a tidy lair.
IndexMap hops into the stream,
The bridge now follows a cleaner dream.
Four lines changed; the burrow beams.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes deferring Dynasm bridge constant conversion to the diagnostic path.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 4ba378b).
Updated: 2026-08-17T12:02:23.044Z

Files in the reviewed diff
majit/majit-backend-dynasm/src/runner.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

None.

@youknowone youknowone left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you so much! and welcome to Pyre project!

@youknowone
youknowone merged commit 81e8a2d into youknowone:main Aug 17, 2026
17 of 19 checks passed
@notJoon
notJoon deleted the refactor/lazy-dynasm-bridge-constants branch August 17, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants