Skip to content

eqPtrs: see through array cells; linear pointer collection; correct the "tsorted" claim#6

Open
nanavati wants to merge 4 commits into
mainfrom
claude/eqptrs-array-cells
Open

eqPtrs: see through array cells; linear pointer collection; correct the "tsorted" claim#6
nanavati wants to merge 4 commits into
mainfrom
claude/eqptrs-array-cells

Conversation

@nanavati

@nanavati nanavati commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Note: intended for B-Lang-org/bsc main (the automation could push the branch but not open a cross-repo PR into the upstream org); opened here to be moved. Upstream compare link: B-Lang-org/bsc@main...nanavati:bsc:claude/eqptrs-array-cells


ICLazyArray elements are heap pointers hidden inside a value constructor (ArrayCell — see its comment in ISyntax.hs: elements are guaranteed to be references so that improveIf's equality check is a pointer comparison). That makes them invisible to expression-shaped traversals, and eqPtrs — the module-output CSE over heap defs — had two such blind spots:

  • hptrs (the internal tsort's edge function) emitted no edges from a residual PrimArrayDynSelect's array to its element cells, so the CSE fold could process a selection before its elements and the non-circularity check did not cover array-mediated references.
  • The CSE substitution did not canonicalize element pointers, so two selections over element-wise CSE-equal arrays never compared equal (cmpC compares arrays by ac_ptr) and always produced duplicate defs in the IModule.

Both get arms. The substitution arm affects only the comparison key, exactly like the existing IRefT arm: emission still goes through the caller's pointer-translation map, which already translates array cell pointers one by one (hToDef converts ICLazyArray to PrimBuildArray over translated refs), so a merge can never produce an inconsistent reference — it only redirects a duplicate def to its representative. The new tsort edges stay within the collected pointer set (collPtrs descends ArrayCells with the same reach), and an array-mediated reference cycle would already fail during normalization, so the edges cannot reject a previously-working design.

While touching hptrs, the quadratic Data.List.union folds are replaced with a single IntSet-backed collector. The collection deliberately preserves first-occurrence order: the tsort's output order is sensitive to edge order, the CSE fold's processing order follows it, and that decides — between duplicate defs — which pointer becomes the representative whose number appears in generated def names, which surface as signal names in the generated code. Preserving the order is essentially free (one extra membership test and one cons per pointer versus dumping the set sorted) and is a convenience, not a contract.

This is hygiene, not a user-visible bug fix. An A/B probe (two selections with the same index over separately-constructed, element-wise identical vectors, let-bound early and forced inside rules) shows exactly what the blindness costs and doesn't cost:

  • Before: the IModule carries two textually identical selection defs; after: one. The final Verilog is byte-identical either way — downstream CSE cleans the duplicate.
  • The same probe demonstrates that the defs list can order a selection def before its element defs (an HUnev cell allocated early, filled late) — harmless, because it turns out the emitted defs list is not in topological order for anyone and never has been: eqPtrs returns M.elems of the expression-keyed CSE map, i.e. canonicalized-expression order; the tsort's order is consumed only by the internal fold. The comment at the call site ("The pointers are returned in tsorted order") and the variable name tsorted_cse_ptrs promised otherwise; a follow-up commit corrects both so no future pass author builds on a dependencies-first assumption that does not hold.

Testing

  • Full testsuite on this branch: 18,501 PASS / 0 unexpected FAIL / 129 XFAIL (the only failures in the test environment are 20 SystemC tests without libsystemc and 3 permission-denied tests that cannot fail when run as root; identical to the unmodified baseline).
  • The def-name-sensitive directories (bsc.scheduler/urgency, bsc.verilog including the Mips sim, and all five bsc.arrays suites) pass with zero golden churn, confirming the order-preserving rewrite changes no generated output.
  • The A/B probe above: IModule def dumps and generated Verilog compared against unmodified main.

Split out as a standalone change because it is independent of any feature work: it was found while auditing heap-pointer-bearing value constructors (the ArrayCell pattern) for B-Lang-org#1008, but nothing here depends on that PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj


Generated by Claude Code

claude and others added 4 commits July 8, 2026 10:25
Array elements are heap pointers hidden inside a value constructor
(ArrayCell -- see the comment in ISyntax), so eqPtrs' expression
traversals were blind to them in two places:

  * hptrs (the def tsort's edge function) emitted no edges from a
    residual dynamic selection to its element cells, so the tsorted
    def order never constrained array elements relative to their
    consumer, and the non-circularity check did not cover
    array-mediated references.
  * The CSE substitution did not canonicalize element pointers, so
    two selections over element-wise CSE-equal arrays never compared
    equal (cmpC compares arrays by ac_ptr) and always produced
    duplicate defs.

Both get arms.  The substitution arm affects only the comparison key,
exactly like the existing IRefT arm: emission still goes through the
caller's pointer-translation map, which already translates array cell
pointers one by one (hToDef converts ICLazyArray to PrimBuildArray
over translated refs), so a merge can never produce an inconsistent
reference -- it only redirects a duplicate def to its representative.

While touching hptrs, replace the quadratic Data.List.union folds
(and the would-be nub over element pointers) with a single
IntSet-backed collector.  The collection deliberately preserves
first-occurrence order: the tsort's output order is sensitive to edge
order, and downstream def order and CSE-representative choice follow
it, so a sorted collection would churn generated-code order for no
reason.  (Util.fastNub is sort-based, hence unsuitable here.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj
Co-Authored-By: Ravi Nanavati <ravi@matx.com>
The order-preserving collection is essentially free (one extra
membership test and one cons per pointer versus dumping the IntSet in
sorted order); it exists to keep the rewrite from reordering every
generated module's defs, and is a convenience, not a contract --
nothing downstream is entitled to a particular def order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj
Co-Authored-By: Ravi Nanavati <ravi@matx.com>
Demonstrating the array blindness showed that the def list emitted for
an IModule is not in topological order and never has been: eqPtrs
returns M.elems of the CSE map, which is canonicalized-expression
order; the internal tsort only sequences the CSE fold and provides the
non-circularity check that pDef's lazy knot needs.  A probe (two
selections over separately-built identical vectors, let-bound early
and forced late, so the consuming cell's pointer is lower than the
element cells doDynSel allocates when it is finally forced) produces a
def list on unmodified main whose FIRST def references its LAST --
harmless today, because nothing downstream assumes dependencies-first
order, but the call-site comment ("The pointers are returned in
tsorted order") and the variable name (tsorted_cse_ptrs) promised
otherwise to future readers.  Rename it to cse_ptrs, document the
actual order, and make the hptrs comment precise about what
first-occurrence preservation protects: the CSE-representative choice,
hence generated def NAMES; the defs list order is expression-sorted
regardless.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj
Co-Authored-By: Ravi Nanavati <ravi@matx.com>
Def names surface as signal names in the generated code, so a
gratuitous rename shows up in netlist diffs, waveform setups and
constraint files; that -- not the defs list order, which is
expression-sorted regardless -- is what first-occurrence collection
protects.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj
Co-Authored-By: Ravi Nanavati <ravi@matx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants