perf(codegen): version counted loops once at entry — no per-iteration flag or f64 counter shadow - #11053
perf(codegen): version counted loops once at entry — no per-iteration flag or f64 counter shadow#11053proggeramlug wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe loop lowerer adds a guarded i32-counter specialization for eligible dynamic-bound ChangesDynamic-Bound Loop Counter Versioning
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established; the change is ready for normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
Status from the merge-train side: this is one gate away from being landable, but I have held it because the decision about whether it is worth landing looks unresolved. Only failing step on the current head: Everything else is green. That is a one-file fix — What I do not want to do is land it on my own judgement, because the admission numbers recorded during triage were narrow: 5 of 392 counted loops admitted in the tsc workload, 0 of 9 in Zod. The measured per-iteration win is real and the asm diff is convincing, but a tier that admits ~1% of real loops buys little while adding a lowering path that has to stay correct forever — and this codebase has a standing rule against a fast path that is rarely hit alongside a slow path that always is. So, two questions for you as the author:
Tell me which and I will either add the fragment and put it in the next train, or close it with your reasoning recorded. I will not sit on it either way. |
…representation flag or f64 counter shadow A dynamic-bound `for (let k = 0; k < n; k++)` loop tested a representation flag on every iteration and advanced a double copy of the i32 counter on every backedge, although the body never read it. The representation is now decided once before the loop: when the entry guard proves the bound and start are integral i32 values, the loop keeps only the i32 counter and materializes a double where `k` is read and on exit. Strict `<` with a sole `++` means the counter never passes `n`, so it cannot overflow; larger, fractional or non-number bounds run in doubles from entry. Captures, try regions, labels, counter mutation and non-straight-line bodies keep the existing lowering, and the canonical-i32 kill switch also disables this. instructions:u per iteration (1M vs 5M slope, output identical to node): q_par 53 -> 47, q_loc 36 -> 27, p1 55 -> 47, p2 55 -> 47, p3 115 -> 106, p4 432 -> 411, p2m 51 -> 46, p4m 429 -> 411. Covered by an executable + IR test (both canonical-i32 modes), a bounded crossing above INT32_MAX, escaping closures, post-loop reads and `arguments`; sabotaging the fractional or range admission turns the witness red. A function-scoped `var` counter captured by a closure hangs on main before this change and is not admitted here: #11052. Claude-Session: https://claude.ai/code/session_01EQdCw7BN4AAnn2hAbNXg33
e8c3b89 to
ff81b22
Compare
|
Landing this — the owner's call, as groundwork for #10741 rather than on its own merits. What was actually blocking it: the changeset gate, not the change. The fragment existed as I also recorded the admission scope in the fragment, because it is the thing most likely to be misread later:
That matters because the asm diff is genuinely convincing — 5 of 53 instructions per iteration — and someone benchmarking a real program will see nothing move and reasonably conclude the lowering is broken. It isn't; it just almost never fires yet. #10741 is the work that changes that. Nothing else is red: It goes into the next merge train. |
…n scope The changeset gate requires a PR-keyed filename, changelog.d/<PR>-<slug>.md -- it matches ^changelog\.d/[0-9]+-[^/]+\.md$ over the files the PR ADDS, so astra-ivshadow-loop-counter.md was invisible to it. PR-keying is what stops in-flight PRs colliding on one fragment. Also records the measured admission scope, because it is the thing most likely to be misread: 5 of 392 counted loops in tsc, 0 of 9 in Zod. The per-iteration asm win is real (5 of 53 instructions), so someone benchmarking a real program will see nothing move and reasonably conclude the lowering is broken. It is not; it almost never fires yet. #10741 is the work that widens admission, and this is its entry-guard machinery.
|
Landed on main in merge train 270 (#11132, v0.5.1653). The train rebase gives commits new SHAs, so GitHub cannot close this automatically. |
1 participant
Part of the property-read parity campaign. A disassembly of
function run(n,O){let h=0;for(let k=0;k<n;k++)h+=O.a+O.b;return h;}(53 instructions/iteration vs node's ~12, all inline) showed 5 of the 53 spent on the loop counter, not on the reads. Every iteration ran acmpb $0,-0x44(%rbp)representation-flag test, and every backedge advanced a double copy of the i32 counter (vmovapdload,vaddsd 1.0, store) that the body never reads.Change: the representation is decided once, before the loop. When the entry guard proves the start and bound are integral i32 values, the loop keeps only the i32 counter and materializes a double where
kis read and on exit. With a strict<and a sole++, the counter cannot passn, so it cannot overflow. Larger, fractional or non-number bounds run in doubles from entry. These cases keep today's lowering: captures, try regions, labels, counter mutation, and bodies that are not straight-line. The canonical-i32 kill switch disables this too.instructions:u per iteration, taken as the slope between 1M and 5M iterations (min of 3 at each count). Output is byte-identical to node 26 on all 32 runs. A = main f5cfbff (perry sha256 1f1ef6c5fee7…), B = this branch (bb654007a784…):
Validation
--test-threads=1: 1,672 passed, 0 failed, 1 ignoredcrates/perry/tests/loop_i32_versioning.rs(both canonical-i32 modes), covering:n = 2**31+5)argumentsFound while testing, and not caused by this change: a function-scoped
varcounter captured by a closure hangs on main (#11052). This change does not admit that case.https://claude.ai/code/session_01EQdCw7BN4AAnn2hAbNXg33
Summary by CodeRabbit