eqPtrs: see through array cells; linear pointer collection; correct the "tsorted" claim#1030
Open
nanavati wants to merge 4 commits into
Open
eqPtrs: see through array cells; linear pointer collection; correct the "tsorted" claim#1030nanavati wants to merge 4 commits into
nanavati wants to merge 4 commits into
Conversation
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>
Collaborator
|
While there's no change in the Verilog, do we still want to have a test case that dumps an ISyntax stage and confirms that CSE happened early? I'm not immediately understanding the comments in the code, so I'll have to come back and review this when I have time -- which might be an indication that the comments could be clearer or more concise. (The PR description was also a wall of text that was hard to decipher. To be able to quickly triage a PR, it would help to put the big picture up front in an intro: what's the motivation; is it user-visible, is it a bug fix, or is it just a clean-up; etc.) |
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.
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 #1008, but nothing here depends on that PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01L1JGkCsP4oRfKg5bEJecVj