jit: #1093 review follow-ups on the operation dump, the slice-index match, and the closure-select fixtures - #1110
Conversation
…atch, and the closure-select fixtures no_source_lines takes the failing operation's index and ends its 40-op window there, so a block whose failure sits past the window still shows that op and the ops that produced its operands; the elided prefix is reported as "… N earlier operation(s)". All five call sites pass the index. A 60-operation regression test with operindex 55 asserts op[55] and op[54] are listed, op[56] is not, and the listing stays 40 long. rewire_one_slice_index_rangeto_site's nested `if let` plus inner `match` becomes a single `match`, which removes the MinusOne unreachable!(). call_function_impl_result_has_no_residual_array_index asserted that the residual index call remains, so it is renamed call_function_impl_result_keeps_residual_array_index and its doc comment is corrected. The comment claimed an index_mut call that this graph does not contain; the graph's array/slice calls are core::array::<Impl>::index x1, __array_repeat x1, core::slice::iter x4 and extend_from_slice x2. The paired negative __getslice_rangeto == 0 is asserted instead. option_closure_select's fixtures give the result enum a different owner root (test::result::Option) from the receiver's (test::recv::Option), and the tests assert FieldWrite::owner_root for __discriminant and __pos_0. Two tests cover result_niche: true, asserting no synthetic constructor, no __discriminant write and one core::ptr::null_mut call. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 633e6b1). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
1 participant
Follow-up to #1093, which merged before its review tail was closed. One commit,
majit-translateonly.Seven findings were open on #1093. Five are fixed here, one was already moot, one is declined with evidence.
Fixed
tool/error.rs— the operation dump could omit the failing operation.no_source_lineslisted the first 40 operations of a block. A block whose failure sits past operation 40 therefore showed neither the failing op nor the ops that produced its operands, which is the entire point of the dump. It now takes the failing operation's index and ends the window there, reporting the elided prefix as… N earlier operation(s). All five call sites pass the index.A 60-operation regression test with
operindex = 55assertsop[55]andop[54]are listed,op[56]is not, the listing stays exactly 40 long, and both elision lines read… 16 earlier operation(s)/… 4 more operation(s). The fixture setsgraph.func = Noneso every source accessor errors and the dump path is the one under test.front/slice_index.rs— nestedif let+ innermatch.Collapsed into a single
match, which removes theSliceIndexBounds::MinusOne => unreachable!()arm. The twounreachable!()s at:214and:673are unrelated (they re-destructure an already-matchedOpKind::Call) and are left alone.front/mir.rs— the anchor's name contradicted its body.call_function_impl_result_has_no_residual_array_indexasserted that the residual index call remains. Renamed tocall_function_impl_result_keeps_residual_array_index.The review also asked for an
index_mutassertion. I probed the real graph rather than adding it blind:call_function_impl_resultcontains noindex_mutat all. Its array/slice calls arecore::array::<Impl>::index×1,__array_repeat×1,core::slice::iter×4 andVec::extend_from_slice×2. The request and the pre-existing doc comment were both unfounded, so the comment is corrected and the verifiable paired negative__getslice_rangeto == 0is asserted instead — the fold is declined, so no site may be rewritten.front/option_closure_select.rs— the fixtures could not distinguish the two owners.Receiver and result shared one owner root, so an assertion about "the result enum" would have passed against the receiver's. The fixtures now use
test::recv::Optionandtest::result::Option, and the tests assertFieldWrite::owner_rootfor__discriminantand__pos_0.Mutation-proven rather than asserted-and-hoped: swapping
result_option_ownerback tooption_ownerfails the test withleft: ["test::recv::Option", "test::result::Option"]vsright: ["test::result::Option"].The
__pos_0owner list has two entries — the second__pos_0write is the closure's(x,)Argstuple fromemit_call_once, a realTupleunrelated to eitherOptioninstantiation. That is asserted explicitly so the extra entry cannot be mistaken for leakage.front/option_closure_select.rs—result_niche: truewas never exercised.Two tests added for
mapandand_then, asserting no synthetic constructor, no__discriminantwrite, and exactly onecore::ptr::null_mutcall. They cannot pass on the non-niche path.Moot
slice_index.rs:920's== falsepredicate was already removed bye1fc067eae2.Declined
flowspace_adapter.rs:4740— the suggestion was to change the fixture'sValueType::Int.front::mir.rs'sRvalue::Repeatreally does emitresult_ty: ValueType::Intfor__array_repeat, andgit show 128590c675fconfirms that declaration predates this work, so the fixture mirrors what it models. Changing it would make the fixture disagree with the lowering. The graph-level contract is anchored by the sibling real-LLBC test directly below it.Verification
cargo test --release -p majit-translate --lib— 3132 passed, 0 failedcargo test --all --no-default-features --features dynasm— 102 suites, 7521 passed, 0 failed (run at basefd658cb0723; the base has since advanced by jit(wasm): drop the wasm32 arm of the self-recursive root-bridge inline #1106, which this commit does not touch)cargo fmt --checkclean— authored by Claude