Repository navigation
[AUTOMATED] Cache full IR keys for inline arena checks - #1107
Conversation
Share compact comparator snapshots and precompute address and flag ordering. Preserve creation order, SeqNum quirks, and unification failure states. IR instructions fall 7.06% with identical output; 66 focused bank and Funcdata checks pass.
Pack signed size and class ordering into one location-key prefix. Reuse immutable address and size snapshots when a node changes class, and construct birth keys directly. Immediate teardown clears the live definition without inserting a temporary free entry before removing the indexed snapshot. Preserve combined class masks, signed bounds and SeqNum's uniq-only equality; add comparator, range, snapshot and teardown coverage and update the spec. Focused validation: 127 unit/integration tests, test layout, spec anchors and diff checks passed. Final candidate reduces instructions:u by 8.16% on ir and 9.07% on core against edba009, with identical outputs; smoke also matches. Three fresh core CPU pairs remain pending at the user's requested pause.
Keep SlotMap storage and ID allocation while validating Varnode and op accesses against a compact array of full FFI keys. Invalidate cached keys before removal or clear, and retain SlotMap validation on debug reads. Expose the checked bank accessors and two hot record getters to inlining. The IR panel uses 5.56% fewer user instructions and smoke uses 6.98% fewer; all outputs are identical. All 52 focused arena/bank tests and the spec, test-layout, formatting, and diff checks pass. Core measurements continue.
|
[AUTOMATED] I re-measured the arena commit (b1259a4) alone, rebased onto main The output is unchanged. Speed was measured on an Intel Xeon Silver 4316 (Ice Lake-SP) with
We can't read perf counters here, so I can't check the instruction counts. The −5.6% instructions you measured may well be real, but on this CPU the change costs cycles. We would merge it with a measurement that shows a gain on an Intel server part, or with a variant that is neutral or better here. |
|
[AUTOMATED] Re-measured after main gained #1203 (FxHash id maps), on this PR merged with main abace00. Output is byte-identical (60 binaries, |
Resolve docs/spec/00-overview.md: keep main's block-order and delete_varnode paragraphs, then the checked-arena paragraph. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013BBaWTixHX2E5cBwneY5aW
The separate key array made lookups about 2% slower in user CPU on an Ice Lake-SP Xeon. Every lookup loaded one more cache line, and it had to build the 64-bit key from its two halves first. SlotMap's version word already sat next to the flag words that most lookups read. Each slot now stores its full key word directly after the value, so the inline check reads a line the caller already touches. Allocation, free-list order and versions replicate slotmap::SlotMap, so ids and output are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013BBaWTixHX2E5cBwneY5aW
|
[AUTOMATED] I pushed two commits: a merge of current main, and 3e42e5c, which moves each cached key out of the side array and into its slot, directly after the value. Why: on our Xeon Silver 4316 (rustc 1.90), the side-array version measured about 1.6-2% more user CPU in five separate reviews. Main already inlines Repro (user CPU vs main, interleaved, each run on an idle physical core):
If you can re-measure on the machine where you saw the gain, we'd like to know whether it still holds. |
SPARC32 callers of libgcc's `__divdi3` decompile with four 32-bit arguments and an unsigned, word-swapped quotient:
```sh
printf '%s\n' 'void g(void);' \
'long long q(long long a,long long b){return a/b;}' \
'long long keep(long long a,long long b){long long r=a/b;g();return r;}' \
'long long guard(long long a,long long b){if(b==0)return -1;return a/b;}' \
> /tmp/di-returns.c
clang -target sparc-linux-gnu -integrated-as -O2 -c /tmp/di-returns.c -o /tmp/di-returns.o
kuna decompile-all /tmp/di-returns.o --option libgccsigs on
```
Default output is `unsigned long long keep(...) { unsigned long long v1; // o1:o0 v1 = __divdi3(a0,a1,a2,a3); ...`. With `--option libgccsigs on`, `q`, `keep` and `guard` return `long long` from `__divdi3(CONCAT44(a0,a1),CONCAT44(a2,a3))`; `keep` keeps the intervening call and `guard` returns `-1` without dividing.
- `libgccsigs` (default off) seeds `long long __divdi3(long long, long long)` on SPARC32 big-endian ELF images that name `__divdi3`; DWARF and `--assert` declarations keep precedence. It stays opt-in because callers that test or clamp the quotient before returning it still lose words.
- On high-word-first 32-bit pair models (SPARC32, PowerPC32, MIPS32 BE, ARM/Thumb BE), a caller's return of a declared eight-byte integer call result is kept whole and in ABI order when nothing but the RETURN (or a later call's unresolved input) reads it: return recovery waits for frame SSA, and contained-output guards on four-byte-word pairs are split into per-word trials that only the whole-pair check can activate. A `void` caller that stores or tests such a result stays `void`.
- `RuleLoadVarnode` reads `(base + c) | k` as an offset when the base's known-zero bits prove the OR is an addition.
- Known limit: a SPARC32 `-O0` caller without its own debug info that stores and then returns a declared 64-bit result returns only the high word (main printed `void`).
Tests: `libgcc_divdi3` (generated C recompiled and run at `-O0`/`-O2`, option on and off, BE void-caller regressions), `call_result_pair_returns`, `call_wide`, a `tests/stages/` testcase, and the restored ARM stage assertions.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013BBaWTixHX2E5cBwneY5aW
…ents in images mapped low (Noelo-Lab#1264) ARM code compiled `-fPIC` but linked as a fixed-address executable at 0x10000 loses two kinds of reference: function pointers read through the GOT have no xref at all, and a string whose PC-relative displacement happens to be a mapped address gets none either. The raw displacement is filed as a pointer instead. Thumb code loses every one of these, because its literal loads were not recognised. ``` $ kuna xrefs decompiler/crates/kuna-analysis/tests/fixtures/gotxref_arm_le32 --from arm_timers ... 0x110dc data - @0x100f8 ldr r0,[0x10164] 0x110e4 data - @0x100d4 ldr r0,[0x1015c] 0x120f4 data - @0x100dc ldr r6,[0x10160] $ kuna xrefs .../gotxref_arm_le32 --to on_expire # 0 references to on_expire @ 0x10098 $ kuna xrefs .../gotxref_thumb_le32 --to on_alarm # 0 references to on_alarm @ 0x100b0 ``` 0x110dc, 0x110e4 and 0x120f4 are displacements, not pointers. The fix, in `listing/kuna_picpool.rs` with its plumbing in `xrefs.rs`: - In an image with a GOT or `.dynamic`, a pool word is a displacement whenever the PC is added to it, whatever it looks like, and Thumb literal loads (16-bit `ldr`, `ldr.w`) are read too. Static GOT-less firmware keeps the previous rule and cost. - A PC-composed value that lands in `.got` becomes a GOT base, tracked along every edge (switch cases, calls in callee-saved registers, predicated skips) with a meet at joins. A load through the base plus a pool word files a read of the slot and a reference to the slot's word. `poolref`'s pointer row for a consumed word is dropped. - An odd target in code names the Thumb function (bit 0 cleared) when the walk decoded code or knows a function there, or a `$t` mapping symbol covers it; under `$d`/`$a` it keeps its exact address. - Only data and read rows change; the decompiler reads call rows only. On ARM PIC images the new data references can reorder `decompile-all`'s callee-first pass (struct numbering only). Tests: `gotxref_{arm,thumb}_le32` and `picodd_thumb_le32` fixtures with console verify tests, three `tests/cli/` probes, `kuna_picpool` unit tests, spec prose for the shapes still not covered, and the closed `arm-pic-got-xrefs` need. Speed: GOT-less firmware reference walk within ~1% of main; ARM PIC libraries +7-8% walk time for their new rows; no measurable `decompile-all` change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013BBaWTixHX2E5cBwneY5aW
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013BBaWTixHX2E5cBwneY5aW
Stacked on #1088: the first two commits are #1088 unchanged; the change here is the last commit,
[AUTOMATED] Cache full IR keys for inline arena checks.Varnode and PcodeOp lookups by id are the second-largest CPU category on ordinary binaries (14.6% of self cycles on whole Bash), and they repeat: counting public bank
get/get_mutcalls inside Actions on Bash, 2.98 billion lookups were made and 92% of them hit an id already looked up in the same Action call. Each one pays SlotMap's out-of-line generation check. This change makes the check a single inlined compare, so the repeated lookups become cheap without rewriting dozens of rules.Main
4aac7a27: 51.12 G instructions. #1088 plus this commit: 44.75 G (−12.5%), identical JSON; #1088 alone accounts for about half of that.4aac7a27Complete JSON is identical on every measured binary. Bash peak RSS moves 252.7 → 254.0 MiB.
kuna_checkedarena::Arenawraps the SlotMap: SlotMap still owns storage and allocates every id. Next to it, aVec<u64>holds each live slot's full key (index and generation, 8 bytes per slot); a vacant slot holds a marker whose index is never allocated.get/get_mutcompare the requested key with that word and then read the slot withget_unchecked.insert,removeandclearare the only paths that change slot membership, and each keeps the word in step with it (removal and clear invalidate it before touching storage), so a matching word means a live key. A stale, removed or null id still returnsNone, as before.PcodeOp::codeandVarnode::has_no_descendbecome inlinable.The
unsafeis the review point. It relies on one invariant held inside a 100-line module: the cached word equals the key only while that key is live. Debug builds keep SlotMap's own check (get_uncheckedassertscontains_key), and a differential test drives random insert/remove/clear sequences against a plain SlotMap. The safe alternative, the same cache returningOption<Key>with SlotMap's checked access, measured −4.1% instead of −5.6% on the same set.Tests: 326
substrate::unit tests pass, including the new arena differential and index tests and accessor-validity tests for both banks (stale, retired, null and cleared ids).