perf(lift): one page mark per straight-line group of stores through a base register by klimovich008 · Pull Request #912 · NotRexed/Hookshot

klimovich008 · 2026-11-09T01:15:25Z

What this fixes

With A64_WRITE_TRACK, every non-SP guest store marks its page with two release byte stores. Lifted code spends a measurable share of game-thread time on those marks.

Why merge

A group of stores through one base register, in a straight-line run spanning at most 4 KB, now gets one shared A64_MARK. It covers the same two pages, placed before anything that ends the run. Mark stores drop from 2,521,896 to 1,454,508 (−42.3%) on lifted v2.0.6 code.

Change

nx2/lift.py edit_c runs the text patches first, then a64_lift.group_marks once over the written C. This keeps oot_patches.py's exact-text edits intact (the review's blocker).
On the supported build's full lift, each output-size edit must apply, or be already applied, exactly once. Otherwise the lift stops before compiling. Other patches still only warn.
What ends a run: labels, branches, returns, unbalanced braces, STLR/LDAR, and any call not on a short list of plain loads, stores and pure helpers.
chunks.write clears an earlier lift's hot pieces before patching.
A64_MARK_EACH=1 turns grouping off.

Tested

tests/test_mark_groups.py, 21 cases:

patches-then-groups through edit_c, with the old order as a control that must fail;
the patched function compiled and run with and without groups, with the same memory and pages;
address forms, the 4 KB span, a hot rebuild, and duplicate or mixed patch counts.

All 21 deliberate sabotages fail it.

Not covered

test_insn/test_leaf not run (needs Unicorn and a game dump). No full lift or game run of this revision.
The earlier CPU result (68.0 vs 69.7 ms/frame) was a batch build. It doesn't isolate this change.

🤖 Generated with Claude Code

hckrman101

Thanks for this, the mark reduction is a nice win. I went through it against the lifted C and most of it holds up well:

The run ends cover every call/sync point that actually shows up in Hookshot's lifted C. nx2/insn.py already turns STLR/LDAPR/LSE into __atomic_* and DMB/ISB into recomp_barrier, so those all hit MG_BOUNDARY.
Pre/post-index writeback stores keep their own marks, since the base write on the same line keeps them out of a group.
The lo/hi math, the Q/STP sizes, negative offsets and the exactly-4096 span all look right. The group's A64_MARK covers the same pages the individual marks did.
A64_MARK_EACH skips the pass entirely, and the lift key already includes the A64_* env vars, so switching it rebuilds cleanly.

One blocker though: as written this breaks the Output-resolution patch in nx2/oot_patches.py. Details are inline. It needs fixing before merge. The rest are smaller robustness and cleanup notes.

hckrman101 · 2026-11-09T16:54:16Z

+        if i in unmark:
+            for ms in sorted(unmark[i], reverse=True):
+                m = MG_STORE.match(line, ms)
+                line = line[:m.start()] + "recomp_%s_sp(c," % m.group(1) + line[m.end():]


Blocking: this renames grouped stores to recomp_*_sp and inserts A64_MARK lines at lift time, which is before nx2/oot_patches.py runs. OUT_EDITS matches exact text, and OUT_EDITS[0] anchors on { uint64_t _b=c->x[21]; int64_t _o=20; recomp_store64(c,(_b+_o),c->x[8]); } in f_81222ce0 (nnMain). Once that store is grouped the anchor stops matching. apply_output_size then only prints oot_patches: WARNING: the output size places were found [...] times (oot_patches.py:291-292) and moves on, so the framework create arg stays 2560x1440 and the window textures ignore the Insert menu's Output resolution. I reproduced it by running this head's mark_groups on a cached lifted f_81222ce0. OUT_EDITS[3] (the recomp_store32(c,c->x[19] + 3668,...) store) has the same weakness.

Some options: run mark_groups after the oot patches, make the anchors tolerant of _sp and inserted marks (or have grouping skip patched functions), and make a missing anchor fail the build rather than warn. The last one seems worth doing either way.

Checked at 816a472: the patches run first, then group_marks over the written files, and the supported lift stops if a place is missing. Looks fixed to me.

hckrman101 · 2026-11-09T16:54:17Z

+# address forms (the text right after "recomp_X(c,")
+MG_ADDR = [
+    (re.compile(r"c->x\[(\d+)\] \+ (-?\d+),"), "x"),          # c->x[N] + IMM,
+    (re.compile(r"c->x\[(\d+)\],"), "x0"),                     # c->x[N],


The c->x[N], form only comes from suyu's STLR, which is emitted as a plain recomp_storeN(c,c->x[rn],...). LDAR is a plain load too, and neither one ends a run. So in the vendored kit without nx2/insn.py, a store-release gets grouped and the marks of earlier data stores get pushed past it. That goes against the "before any guest synchronisation" guarantee in the comment above. Hookshot itself is only safe because nx2/insn.py rewrites STLR into __atomic_store_n. Could this form end the run instead, or more generally any store-release/load-acquire?

hckrman101 · 2026-11-09T16:54:17Z

+MG_BDEF = re.compile(r"uint64_t _b=c->x\[(\d+)\]; int64_t _o=(-?\d+);")
+MG_ADEF = re.compile(r"uint64_t _a=c->x\[(\d+)\];")
+# a line that ends the straight-line run: control flow, calls, polls, any synchronisation or runtime call
+MG_BOUNDARY = re.compile(r"^\s*L_[0-9a-f]+:|\bgoto\b|\breturn\b|TAILCALL|\bswitch\b|\bf_[0-9a-f]{8}\(|\bA64_[A-Z_]+\(|"


There's no brace tracking here. ^\s*\}\s*$ is meant to catch the function end, but it also matches the closing line of any multi-line instruction block. A line that opens a block with if/for/while doesn't end the run either. So a deferred mark can land inside an enclosing block. In OoT's lifted C about 140 group marks end up inside unconditional FMUL/FMLA scopes, which is harmless today. But if a conditional multi-line emission ever showed up, marks for earlier unconditional stores would become conditional. Ending the run on any line containing { or }, or tracking depth, would close that off.

hckrman101 · 2026-11-09T16:54:17Z

        if frame:
            text = frame_stores(text)
+        if not os.environ.get("A64_MARK_EACH"):
+            text = mark_groups(text)


Bigger picture: doing this as a regex pass over the finished C text is what causes the two issues above, and the token list has to be kept in sync with every helper. Grouping inside emit_function's per-instruction loop would avoid that: kind != 'op' or pc in labels ends a run, and reg_writes(text) tells you when the base is written.

Separately, mark_groups is called here without stats, so the groups/grouped_stores counters never make it into the lift report. Passing self.stats would fix that.

hckrman101 · 2026-11-09T16:54:17Z

+# a line whose stores may not all run, or run more than once: its stores keep their own marks
+MG_NOT_STRAIGHT = re.compile(r"\bif\b|\?|\bfor\b|\bwhile\b|\bswitch\b|\bcase\b")
+# (any write of a guest register: =, compound assignments, ++/--, or its address taken (LDP's outputs))
+MG_REG_WRITE = re.compile(r"c->x\[(\d+)\]\s*(?:[-+*/%&|^]|<<|>>)?=(?!=)|&c->x\[(\d+)\]|(?:\+\+|--)\s*c->x\[(\d+)\]|"


Nit: MG_REG_WRITE duplicates REG_WRITE/reg_writes() further down (plus ++/--). It'd be nice to have one definition so the two can't drift apart.

hckrman101 · 2026-11-09T16:54:17Z

+    return None
+
+
+def mark_groups(text, stats=None):


Minor perf: this runs the ~30-alternative regexes on every line of every function. I measured about 1.33 s per 12 MB chunk, roughly 170 s extra over the full lift. An early if len(MG_STORE.findall(text)) < 2: return text skips about 79% of functions.

hckrman101 · 2026-11-09T16:54:17Z

+    return a64_lift.mark_groups("\n".join((HEAD,) + body + ("}",))).split("\n")[1:-1]
+
+
+class MarkGroupsTests(unittest.TestCase):


Test coverage gaps worth filling: (c->x[N]+IMMULL) Q stores, the second Q of an STP at (_b+_o)+16, _a+IMM ST1, negative _o (STUR), an exact 4096-byte span crossing two pages, the STLR c->x[N], form, multi-line blocks, the A64_MARK_EACH switch, and a check that the oot_patches anchors still match after grouping (which would have caught the blocker).

Coverage looks good now. The order tests don't actually run in CI though (lz4 import), see the review.

… base register A64_WRITE_TRACK marks the page of every non-SP store, two byte stores and the address arithmetic each. In a straight-line run (no label, branch, call, return, poll, barrier, atomic, exclusive, SVC or trap) the stores through one base register plus constants spanning at most 4 KB now share one A64_MARK of their byte span, placed after the group's last data store and before the base register changes or the run ends. The pages marked are the same, and each data store is still followed by a release store of its page's mark before any guest synchronisation. 43% fewer mark stores in the lifted game, about 24 MB less code. A64_MARK_EACH=1 at lift time keeps a mark per store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…edit stops the lift Review of NotRexed#912. The groups changed the lifted text before nx2/oot_patches.py matched it exactly: OUT_EDITS[0] (nnMain's create arg) no longer matched, apply_output_size only warned, and the Output resolution was ignored. - Order: emit_function no longer groups. nx2/lift.py edit_c applies the patches, then a64_lift.group_marks over the written code_*.c files, last, once, with the lift's stats (reported). The kit's own write() keeps a mark per store; A64_MARK_EACH=1 still turns the groups off (in the lift key). - Required edits: on the supported build's whole standalone lift, every OUT_EDITS place must be patched, or found patched, exactly once, or the lift stops before the key is written. Other builds, partial lifts and the other patches warn as before. - Run ends, conservative: a line whose braces do not balance (no mark moves into or out of a multi-line block), STLR/LDAR in suyu's spelling, and any call-like token not on a short list of plain loads, stores and pure helpers (f_*, hos_* from the patches, A64_*, atomics, unknown helpers). - The pass is idempotent (its own marks are not run ends), takes the nearest _b/_a definition on a line, skips functions with local registers, and shares REG_WRITE (now with ++/--) with reg_writes. About 6x faster than before: functions with fewer than two stores skipped, lines with no open group skipped, cheap substring tests before each regex. - Lifted v2.0.6 C: mark stores 2,521,896 -> 1,454,508 (42.3% fewer; the first version 43.1%). Idempotent on all 80 files. - tests/test_mark_groups.py: the patches-then-groups sequence through lift.edit_c with the OUT_EDITS places (no game code), the old order as a failing control, the patched function compiled and run with and without groups (same memory, same pages), the review's address forms, the 4096 span, STLR/LDAR, blocks, host calls, A64_MARK_EACH, the lift key, stats, idempotence, and a random page-set check. nx2/test_leaf.py groups its functions as the lift does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nt every made and raw output-size place Review of NotRexed#912 (review229). - chunks.write removes an earlier lift's code_hot_*.c and code_hot.c.whole whether or not this lift writes a code_hot.c. With a hot list, the old pieces (patched already) stayed until make_game's split_hot, after the lift: nx2/oot_patches.py and the groups read them beside the new code_hot.c, an output-size place counted twice, and the required check failed a normal rebuild. split_hot makes this lift's pieces after it. - apply_output_size counts each made place and each raw place outside a made one (a new text can hold its old one), per file. A file with a place made twice, or made and raw, was counted once and accepted; now it is not once, and the supported build's lift stops. A file made once is accepted and left as it is. - tests/test_mark_groups.py: a rebuild over the first lift's patched hot pieces (chunks.write, edit_c, split_hot, twice) finds each place once; made once accepted; made twice, made and raw ([3], and [4] whose new text holds its old one), and raw in another file rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

klimovich008 · 2026-11-09T18:43:17Z

Thanks, all addressed in 3fa6300 and 816a472 (rebased on current main):

Blocker: the lift no longer groups. nx2/lift.py edit_c runs oot_patches, then a64_lift.group_marks once over the written code_*.c. On real v2.0.6 C taken back to its unpatched text, the old order loses OUT_EDITS[0] and the new order patches it.
Required edits: on the supported build's whole standalone lift, each OUT_EDITS place must be found exactly once, or the lift stops before its key is written. Other builds, --max-func and the other patches only warn.
Counting: made places plus raw places outside made ones, per file. Made twice, or made and raw, is rejected; made once is accepted.
Hot rebuilds: chunks.write removes the earlier lift's code_hot_*.c before any patcher runs, so the old patched pieces aren't counted again.
STLR/LDAR: recomp_storeN(c,c->x[N], and recomp_loadN(c,c->x[N]) end the run.
Braces: a line whose braces don't balance ends the run. One-line { _b ... } blocks still group.
Calls: any call-like token outside a short list of plain loads, stores and pure helpers ends the run (f_, hos_, A64_*, atomics, unknown helpers).
Stats: L.stats is passed in and printed by report(). There's one REG_WRITE (now with ++/--).
Perf: about 6x faster (0.93 s vs 5.7 s per 24 MB chunk), roughly 55 s per full lift.
Tests: your list, MARK_EACH, idempotence, a page-set check, a hot rebuild, the count cases, and the patched function compiled and run with and without groups. The old order is kept as a failing control, and 21 sabotages each fail.
Benefit: 2,521,896 to 1,454,508 mark stores on v2.0.6 C (-42.3%, vs -43.1% before).

Not rerun here: test_insn and test_leaf (no capstone/unicorn or game), and no game run.

hckrman101

Thanks, this is a lot better. Went through it again at 816a472:

Ordering: edit_c runs the patches and then group_marks over the written code_.c, so OUT_EDITS see the lift's own text. The made/raw count, and failing only on the supported build's whole standalone lift, look right to me. make_game deletes code_.c before a relift and the lift key covers nx2/, the kit and the A64_* env, so grouped and ungrouped C can't get mixed. Running group_marks twice is a no-op since _sp stores and its own marks are skipped.
STLR/LDAR, the brace check, unknown calls ending a run, the single REG_WRITE, the early return and the stats: all good. Clearing the old hot pieces in chunks.write is fine as well, since split_hot makes the new ones afterwards.

One thing left: the new test fails in CI on both runners. OutputSizeOrderTests imports nx2/lift.py, which pulls in nso.py and lz4, and the runners don't have it. So the three tests that cover the patch/group order (the actual fix) error out instead of running. Stubbing lz4 the way lift_driver() already stubs capstone, or skipping when it's missing, would fix it. (test_output_resolution fails on main too, so that one isn't yours.)

Nits, not blocking:

Lift knobs that rewrite register text (A64_LOCAL_REGS) now stop the supported lift instead of warning, because OUT_EDITS won't match. Probably what you want, just worth knowing.
chunk_costs.txt is written before grouping, so the costs come out slightly high. Doesn't really matter.

I'll approve once the new test passes in CI.

hckrman101 · 2026-11-09T19:08:14Z

+    except ImportError:
+        import types
+        sys.modules["capstone"] = types.ModuleType("capstone")
+    import lift


This pulls in nso.py -> import lz4.block, and the CI runners don't have it, so test_the_patches_then_the_groups, test_a_rebuild_over_an_earlier_lifts_hot_pieces and test_a_missing_place_fails_once_required fail with ModuleNotFoundError on both ubuntu and windows. Stub lz4 / lz4.block like capstone above, or skip when it isn't importable.

nx2/lift.py imports nso.py, which imports lz4 for NSO decompression; the Python source checks have no lz4, so the three tests through lift.edit_c failed at import. They read no NSO: lz4 is stood in for like capstone. (test_output_resolution's hos_rt_pool_size assertion fails on main too.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…s tools The order tests imported nx2/lift.py for edit_c, and lift.py imports the whole lifter at load time (capstone, nso.py's lz4, adr_entries' numpy): the CI runners have neither lz4 nor numpy, so the three tests of the patches-then-groups order errored out. edit_c (the patches, then the groups, over the written code_*.c) now lives beside chunks.write, which already writes those files and imports a64_lift; lift.py calls chunks.edit_c. The lz4/capstone stand-ins in the test go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

klimovich008 · 2026-11-09T19:58:35Z

Thanks. Fixed in 1c5258f: stubbing lz4 got past that import, but lift.py also pulls in numpy (via adr_entries) and the rest of the lifter at load time. So edit_c now lives in nx2/chunks.py, beside chunks.write, which already writes those files and imports a64_lift. lift.py calls chunks.edit_c, and the tests import chunks directly with no module stand-ins. CI is green on both runners now, and the three order tests run there.

On the nits: yes, the supported lift stopping under A64_LOCAL_REGS is intended, since its output-size edits can't apply then. And the costs from before grouping in chunk_costs.txt are fine as a slight overestimate.

hckrman101

Moving edit_c into chunks.py is a cleaner fix than stubbing, and the tests now exercise the real function without stand-ins. CI is green, and the patches-then-groups order and the required output-size check are unchanged. LGTM.

klimovich008 mentioned this pull request Nov 9, 2026

perf(fpx): keep the hardware result for FMUL/FMLA S when the flags are already set #917

Merged

hckrman101 reviewed Nov 9, 2026

View reviewed changes

klimovich008 and others added 3 commits November 9, 2026 19:42

klimovich008 changed the title ~~Lift: one page mark per straight-line group of stores through a base register~~ perf(lift): one page mark per straight-line group of stores through a base register Nov 9, 2026

klimovich008 force-pushed the perf-store-mark-groups branch from b7d5a93 to 816a472 Compare November 9, 2026 18:43

hckrman101 reviewed Nov 9, 2026

View reviewed changes

klimovich008 and others added 2 commits November 9, 2026 20:18

hckrman101 approved these changes Nov 9, 2026

View reviewed changes

hckrman101 merged commit 9c74f8f into NotRexed:main Nov 9, 2026
10 checks passed

		return a64_lift.mark_groups("\n".join((HEAD,) + body + ("}",))).split("\n")[1:-1]


		class MarkGroupsTests(unittest.TestCase):

Conversation

klimovich008 commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this fixes

Why merge

Change

Tested

Not covered

Uh oh!

hckrman101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klimovich008 commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hckrman101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klimovich008 commented Nov 9, 2026

Uh oh!

hckrman101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

klimovich008 commented Nov 9, 2026 •

edited

Loading

klimovich008 commented Nov 9, 2026 •

edited

Loading