perf(codegen): narrow the x|0 i32-slot lever to stable-packed loops - #8771
Merged
Conversation
Lands the follow-up commit from #8719, rebased onto current main. #8719's main body already landed via #8755. This is its remaining commit, cherry-picked so it carries only its own content: merging the branch would have reverted #8740, #8742, #8743 and #8763, whose changes its stale base predates. The lever now additionally requires `!ctx.stable_packed_loop_facts .is_empty()`, because other loop clones own narrower indexed-load contracts that their existing assignment lowering must continue to see. The conflict with the landed soundness fix was comment-only -- the code auto-merged -- and both halves survive: the unproven path still lowers through `lower_expr` + `toint32_fast` and so still reaches `js_dynamic_bitxor`, while the gate is narrowed. The negative control `char_code_at_on_an_unproven_receiver_keeps_the_ runtime_lowering` is byte-identical to main (blob 86b697d) and all three char_code_at probes pass. Known coverage note: those three probes bound their loops with `i < 64` rather than an array length, so under the narrowed gate they no longer enter the branch #8755 fixed -- they still pass, but via the generic tail. That branch's end-to-end coverage is `read_only_loops_have_preheader_ proofs_and_fallback_free_fast_blocks` in `issue_8690_loop_versioned_ arraylike.rs`, which this commit extends but which could not be executed here: it needs the compiler plus the -static wrappers, and the volume would not hold them. No version bump.
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
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
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.
Lands the remaining commit from #8719, rebased onto current
main.#8719's main body already landed via #8755. This is its follow-up, cherry-picked so it carries only its own content — merging the branch would have reverted #8740, #8742, #8743 and #8763, whose changes its stale base predates. I verified afterwards that the five files those PRs touch are blob-identical to
main.What it does
The lever now additionally requires
!ctx.stable_packed_loop_facts.is_empty(), because other loop clones own narrower indexed-load contracts that their existing assignment lowering must continue to see.The conflict with the landed soundness fix turned out to be comment-only — the code auto-merged — and both halves survive, verified in both directions:
main, the only functional change is the narrowed gatelower_expr+toint32_fast) is restored, so the unproven path still reachesjs_dynamic_bitxorValidation
lint-job checkers passchar_code_at_on_an_unproven_receiver_keeps_the_runtime_loweringis byte-identical tomain(blob86b697d489c8); all threechar_code_atprobes passperry-codegen --lib: 1229 passed, 0 failedperry-runtime --lib(RUST_TEST_THREADS=1): 2674 passed, 0 failedA coverage note worth recording
Those three
char_code_atprobes bound their loops withi < 64rather than an array length, so under the narrowed gate they no longer enter the branch #8755 fixed — they still pass, but via the generic tail. The branch's end-to-end coverage isread_only_loops_have_preheader_proofs_and_fallback_free_fast_blocksinissue_8690_loop_versioned_arraylike.rs, which this commit extends.That e2e suite could not be executed here. It needs the compiler plus the
-staticwrapper archives, and free disk would not hold them — I built the wrappers but the test run did not complete before the volume ran down. So the soundness branch is correct by construction and by IR inspection, but its behavioural coverage is currently unexercised. Worth running on a machine with room.