Align rtyper and metainterp public symbol parity with RPython - #326
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (45)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 e3d5a32). 1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
Implement rtype_builtin_range / rtype_builtin_xrange in rrange.rs and register both in rbuiltin.rs install_default_typers. - nb_args 1/2/3 arms select vstart/vstop/vstep; const step of zero raises TyperError. - AbstractRangeRepr result: step != 0 -> gendirectcall ll_newrange, step == 0 -> ll_newrangest. Non-range result (real list) returns a deferred error citing the unported ll_newlist / ll_setitem_fast. - build_ll_newrange_helper_graph: single block, malloc(RANGE.TO) flavor=gc + setfield start/stop. - build_ll_newrangest_helper_graph: start block guards int_eq(step, 0) raising ValueError, build block mallocs RANGEST + setfield start/stop/step. - emit_gc_malloc / emit_void_setfield graph-op helpers. Assisted-by: Claude
Add RPythonTyper.cache_dummy_values and implement the lazy immortal placeholder allocation for both dummy-value builders (rmodel.py:452-464, rgcref.py:93-104): - DummyValueBuilder::ll_dummy_value mallocs an immortal Struct/Array placeholder (n=1 for _is_varsize) keyed by TYPE in cache_dummy_values; the typer is threaded in at call time since the builder stores only rtyper_id for identity. - DummyValueBuilderGCRef::ll_dummy_value casts the base-instance dummy (getinstancerepr(None) -> DummyValueBuilder over TYPE.TO) to GCREF and memoises it under GCREF; return type changed from Constant to LowLevelValue to match the base builder. - The producer side (Repr.get_ll_dummyval_obj) stays deferred: its only consumers are the unported dict/ordereddict ENTRIES.dummy_obj fields. Sharpen the GCRefRepr.ll_str deferral note to cite the concrete blocker: ll_str is carried upstream by VoidRepr/TupleRepr (gen_str_function)/ string reprs, none of which pyre exposes as an ll_str surface, so the hasattr guard is vacuously false. Assisted-by: Claude
Port the profiler class surface from jitprof.py:16-50: - BaseProfiler (jitprof.py:16-17) marker struct. - EmptyProfiler (jitprof.py:19-50) no-op profiler with the full start/finish/tracing/backend/count/count_ops/get_counter/get_times surface; initialized = true. - Profiler type alias exposing the upstream public name for JitProfiler. - BrokenProfilerData error marker. Classify JitProfiler / JitProfilerSnapshot / ProfilerEventGuard as intentional Rust-side extras in check-rpython-module-parity.py. Assisted-by: Claude
The bare ll_tupleiter(&LowLevelType, Hlvalue) / ll_tuplenext(Hlvalue) functions returned rtuple_deferred and had no production callers — a value-returning function cannot emit the malloc/setfield operations these helpers require. rtuple.py:399-411 is faithfully ported by the graph-builder forms build_ll_tupleiter_helper_graph (malloc ITERPTR.TO + setfield 'tuple') and build_ll_tuplenext_helper_graph (getfield/ptr_nonzero /StopIteration/null-store/return item0), which are the ones newiter / rtype_next actually gendirectcall. Move the upstream citations onto those builders and remove the dead stubs plus their assert-deferred test. Assisted-by: Claude
Eliminate the separate-carrier deviation from dependency.py's Path/Node model: Node.op becomes Option<Op> (None for imaginary nodes), and the ImaginaryNode struct plus the PathNode enum are deleted. Path now stores node indices uniformly (Vec<usize>), including imaginary nodes appended to DependencyGraph.nodes via add_imaginary_node. - Node::new_imaginary(label) builds an op=None node carrying a dotlabel and a fake index; is_imaginary() == op.is_none(); getoperation() returns Option<&Op>; op() unwraps for real-node call sites. - Path::second/last/first/last_but_one return the actual node index at each position (real or imaginary), matching dependency.py:56-72. - set_schedule_priority sets priority on every segment including imaginary ones (dependency.py:100-102), where the old split skipped them; is_always_pure skips imaginary segments (dependency.py:84-86). - Cascade node.op field reads to op()/getoperation() across dependency.rs and vector.rs. Assisted-by: Claude
Add per-rule fire counters to the autogen int-rule mixin, mirroring the generated `_rule_names_<op>` / `_rule_fired_<op>` class attributes and their `_all_rules_fired` registration (autogenintrules.py:18-22): - `RULE_NAMES_*` / `RULE_FIRED_*` static tables plus `all_rules_fired()` registry, and a `fire(counts, index)` helper for the in-line bumps. - Bump the matching counter at each of the 136 `optimize_INT_*` rewrite sites (`self._rule_fired_<op>[i] += 1`). - Implement `print_rewrite_rule_statistics` (intbounds.py:862-870): dump the counts in a `jit-intbounds-stats` debug section. - Registry-shape and debug-dump smoke tests. Assisted-by: Claude
Port the dependency-graph path/edge primitives that `analyse_index_calculations` needs (dependency.py): - Node::provides()/depends() — forward/backward edge lists (246-253). - Dependency::target_node()/origin_node()/is_failarg() (429-461). - DependencyGraph::iterate_paths() — index-based port of the Node.iterate_paths generator (303-352): worklist path enumeration with optional destination, direction, path_max_len cap, and blacklist. - Diamond-DAG unit test covering forward/backward/max-len enumeration. Narrow the vector.rs analyse_index_calculations deferral note: of the four cited blockers, imaginary_node (via add_imaginary_node), iterate_paths, and is_always_pure are now ported; only edge_to / remove_edge_to graph-mutation remain, plus #175 for mark_guard failargs. Assisted-by: Claude
Port analyse_index_calculations (vector.py:529-583): build the loop dependency graph, add an imaginary early-exit node, and reroute every valid guard's edges through it so guards fail early, returning the rewired graph as the reschedule basis. Add the graph-mutation primitives it needs to dependency.rs: - edge_to/remove_edge_to (index-based Node.edge_to/remove_edge_to), keeping the deps/users side-vectors the scheduler reads consistent. - Dependency::new regains the failarg argument (dependency.py:415-421); add_edge was dropping it on newly created edges. - schedule_operations excludes imaginary nodes from its output and indexes in_degree by node position rather than the Node.idx sentinel. mark_guard's failargs body stays deferred to #175. Assisted-by: Claude
mark_guard attaches a CompileLoopVersionDescr to GUARD_TRUE/GUARD_FALSE, copying resume attributes from an existing descr, and sets the loop label args as failargs via getarglist_operand. - compile.rs: add make_compile_loop_version_descr / _typed / _with_payload. - dependency.rs: set_edge_failarg propagates a failarg downgrade to both the forward and back edge (dependency.py:457-458 dep.backward).
rtype_builtin_range list-result path lowers to ll_range2list through a helper graph using ll_newlist and setitem. - rlist: expose build_ll_newlist_helper_graph and item_lowleveltype.
build_ll_dictiter_helper_graph, ll_dict_lookup, and ll_dictiter.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 495ec10e82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| guard_op.setdescr(descr); | ||
| } | ||
| guard_op.setfailargs(loop_.label.getarglist_operand()); |
There was a problem hiding this comment.
When an index guard is marked as an early-exit guard, this overwrites its failargs with the loop label args but leaves both op.fail_arg_types and the newly attached CompileLoopVersionDescr carrying the old donor guard's type vector (or an empty vector when there was no donor). For early-exit guards whose original failargs differ from the label args, the backend/deadframe paths decode exits using descr.fail_arg_types(), so guard failures can save or read the wrong number/types of values. After replacing the failargs, also recompute and set the matching fail-arg types on the op and fail descr.
Useful? React with 👍 / 👎.
Summary
Self-review
Assisted-by