Skip to content

Preserve \src attributes through ABC9 via XAIGER y extension#5712

Closed
robtaylor wants to merge 3 commits into
YosysHQ:mainfrom
robtaylor:src-retention-y-ext
Closed

Preserve \src attributes through ABC9 via XAIGER y extension#5712
robtaylor wants to merge 3 commits into
YosysHQ:mainfrom
robtaylor:src-retention-y-ext

Conversation

@robtaylor

@robtaylor robtaylor commented Feb 27, 2026

Copy link
Copy Markdown

Summary

Preserve \src (source-location) attributes on LUT cells through the ABC9 synthesis flow using the XAIGER "y" extension.

  • XAIGER writer (backends/aiger/xaiger.cc): emits "y" identity extension + src map entries in the map file
  • XAIGER readers (frontends/aiger/aigerparse.cc, frontends/aiger2/xaiger.cc): parse "y" extension + src map, apply \src to mapped LUT cells
  • abc9_exe: simplified to use &verify (without -y) for correctness checking only
  • aigmap: propagates \src from replaced cells to new $_AND_/$_NOT_/$_NAND_ gates

How it works

Yosys writes an XAIGER file with a "y" extension containing an identity mapping (each object maps to itself) plus a map file with src lines recording which AIG objects have \src attributes. ABC reads this, propagates the origin mapping through all optimization passes, and writes the updated mapping back. Yosys then reads the output and uses the mapping to look up the original \src for each LUT cell.

Dependency: ABC-side origin tracking

This PR depends on berkeley-abc/abc#487, which adds per-object origin tracking (vOrigins) to ABC's Gia_Man_t.

Without that change, ABC has no mechanism to propagate the "y" mapping through its optimization passes. The identity mapping Yosys writes becomes meaningless after ABC restructures the AIG — objects are renumbered, merged, and eliminated by passes like &dc2, &dch, and the IF mapper. We initially prototyped a &verify -y approach that used combinational equivalence checking to reconstruct the mapping after optimization, but this only achieved 17-57% coverage on non-trivial designs and doesn't work for sequential circuits.

The ABC PR adds a lightweight vOrigins vector that tracks each object's input-side origin as it flows through dup operations, structural hashing, AIG round-trips, and technology mapping — achieving ~100% coverage with negligible overhead (~0.6% time, ~3% memory).

This PR should not be merged until the ABC maintainer has reviewed and accepted the ABC-side changes. The approach and feasibility depend on their feedback.

Coverage

With the ABC origin tracking in place, \src retention on LUT cells achieves 100% on all tested designs (simple combinational, Amaranth-style, and larger multi-output designs with 54 LUTs). Without it, coverage was only 17-57%.

Context

Follows feedback from @Ravenslofty on YosysHQ/abc#41 suggesting the XAIGER "y" extension approach instead of custom node retention infrastructure in ABC.

cc @Ravenslofty — would appreciate your input on this approach. Note the ABC-side dependency at berkeley-abc/abc#487.

Test plan

  • Basic .ys test: \src survives simplemap → abc9 round-trip
  • Full validation: JSON export confirms $lut cells have valid file:line.col \src format
  • Existing abc9.ys test suite passes (no regressions)
  • aigmap \src propagation verified for AND/NOT/NAND paths
  • 100% \src coverage on simple, amaranth, and large (54 LUT) test designs
  • Negligible performance overhead (~0.6% time, ~3% memory on picorv32)

@robtaylor robtaylor force-pushed the src-retention-y-ext branch 3 times, most recently from c88d8c7 to 44ce187 Compare February 27, 2026 12:59
Use ABC's &verify -y mechanism to compute output-to-input object
equivalence mappings during optimization. The XAIGER writer emits
a "y" identity extension and src map entries in the map file. After
ABC optimizes and rewrites the AIG, the XAIGER readers use the "y"
mapping to trace output objects back to input objects, recovering
the original \src attributes for mapped LUT cells.

Changes:
- xaiger writer: emit "y" identity extension + "src" map file entries
- abc9_exe: use &verify -y (combinational) + second &write
- aiger/aiger2 readers: parse "y" extension + src map, apply \src
- aigmap: propagate \src to AND/NOT/NAND gates during AIG mapping

Enabled via: scratchpad -set abc9.verify true

Limitations:
- Combinational only (&verify -y is CEC-only, not sequential)
- Not all LUTs may get \src — only those provably equivalent to
  an input object

Co-developed-by: Claude Code v2.1.44 (claude-opus-4-6)
ABC now propagates origin mappings natively through optimization
passes via vOrigins (berkeley-abc/abc#487), so we no longer need
to run &verify -y and rewrite the output to reconstruct the mapping.

Keep &verify for correctness checking but without -y flag.

Co-developed-by: Claude Code v2.1.44 (claude-opus-4-6)
@robtaylor robtaylor changed the title Prototype: preserve \src attributes through ABC9 via XAIGER y extension Preserve \src attributes through ABC9 via XAIGER y extension Feb 28, 2026
@Ravenslofty

Copy link
Copy Markdown
Collaborator

Including bbdfd0f in this PR seems premature to me without knowing what Alan thinks, and even if a better approach than &verify -y is developed, &verify -y is a thing that exists right now and still provides results that are better than nothing.

@robtaylor

Copy link
Copy Markdown
Author

Including bbdfd0f in this PR seems premature to me without knowing what Alan thinks, and even if a better approach than &verify -y is developed, &verify -y is a thing that exists right now and still provides results that are better than nothing.

that's why it's draft =)
Just keeping you in the loop. Let's see what Alan thinks.

@Ravenslofty

Copy link
Copy Markdown
Collaborator

Well, we know what Alan thinks now.

I think you should revert bbdfd0f because, as mentioned, &verify -y is an approach we can get into Yosys now, and it still provides an improvement over the present state. You can always make a follow-up PR to extend it further if a better approach is hashed out with Alan.

When applying \src attributes from ABC origin tracking, collect source
locations from both the LUT output object's origin and each input
object's origin, merging unique values with '|' (Yosys convention).

This handles the case where a LUT covers logic from multiple RTL
source lines — ABC tracks one origin per object, Yosys merges them.

Co-developed-by: Claude Code v2.1.58 (claude-opus-4-6)
@robtaylor

Copy link
Copy Markdown
Author

Progress update on the ABC side (berkeley-abc/abc#487):

Origin propagation through all ABC9 engines is now implemented and reviewed. Alan Mishchenko has reviewed the approach and confirmed the design is clean. The engines instrumented are: &dc2, &dch, &if, &syn2, &synch2, &sweep, &scorr, &mfs, &nf — covering the full abc9 pipeline.

Performance overhead is within measurement noise (<1%) across all engines.

On the Yosys side, I've also updated this branch to handle multi-source attribution: each mapped cell now collects \src from both its output object's origin and each input object's origin, merging unique values with |. This addresses the case where a LUT covers logic from multiple RTL source lines — ABC tracks one best origin per object, and Yosys merges them at the mapping readback stage.

@robtaylor

Copy link
Copy Markdown
Author

@Ravenslofty Coming back to this — the ABC PR (#487) now has full engine coverage for the abc9 pipeline and Alan has confirmed the approach. Two options:

  1. Split now: Revert bbdfd0f, keep &verify -y as the mechanism, land this PR independently. Origin tracking via "y" extension becomes a follow-up once ABC “converting real value” error message can be more helpful #487 merges.
  2. Wait for ABC to land: Keep the PR as-is, land both together.

Do you have a preference? I'm happy either way — splitting might be cleaner since it decouples the two repos.

@whitequark

Copy link
Copy Markdown
Member

@robtaylor Speaking with my formal @YosysHQ staff hat on for the duration of this comment, and representing the team as a whole: We have discussed this PR internally and decided not to merge it. This is due to both the manner in which it was produced as well as technical issues with it. Since I am far from being an expert on ABC, I will leave the discussion of the latter to @Ravenslofty, who has final say over this part of Yosys.

@whitequark whitequark closed this Mar 11, 2026
@robtaylor

robtaylor commented Mar 12, 2026 via email

Copy link
Copy Markdown
Author

@robtaylor

robtaylor commented Mar 12, 2026 via email

Copy link
Copy Markdown
Author

@ShinyKate

ShinyKate commented Mar 12, 2026

Copy link
Copy Markdown
Collaborator

YosysHQ is not outright against use of LLMs. Our review guidelines state "We do not ban LLM code from the codebase, we ban bad code."

But our contributing guidelines ask that people discuss their motivation and approach ahead of time on the Discourse forum. Especially with such a large change this is what we expect so that we can make sure our goals are aligned and no one wastes time.

In the case of the 2,000 line ABC PR, you did not do this. We at YosysHQ accept responsibility that our downstream ABC repo did not communicate this expectation because we did not expect anybody to file PRs against it and not against upstream ABC; as a result we have explicitly disabled PRs on that repo.

At the time Lofty pointed you to the Yosys Discourse, and you said that you would update it as things progress, but you haven't in two weeks. To underline this need for communication, the Yosys PR template has as its top line "If your work is part of a larger effort, please discuss your general plans on Discourse first to align your vision with maintainers."

While Lofty did suggest that splitting up the PR would have been the preferred approach that alone will not be enough to approve. Between pushing back on feedback because the PR was in a draft and ignoring suggestions for multiple weeks we're losing faith that we will see enough alignment with our quality standards and Yosys style patterns.

We do think this functionality is valuable but we take issue with the implementation and the process in which this was presented. To move this forward I suggest restarting discussion in the discourse thread.

@robtaylor

robtaylor commented Mar 12, 2026 via email

Copy link
Copy Markdown
Author

@Ravenslofty

Copy link
Copy Markdown
Collaborator

The focus of that discussion was on ABC internals. Other than my suggestion that the XAIGER y extension was used, what goes on inside ABC is up to Alan.

@robtaylor

robtaylor commented Mar 12, 2026

Copy link
Copy Markdown
Author

Yes, but it is a dependency of any successful implementation of this functionality.

@Ravenslofty did you see the private email exchange with Alan following on your comment ~two weeks ago?

Also on the abc discussion, Alan is happy with the general approach in that issue, and as long as when the functionality is zero cost when not used, he's flexible. What's really needed here is for us to figure out what we want to do wrt node merging.

My current plan is to make a test set so we can evaluate how much merging happens in common designs, but you will have a deeper understanding here than I.

robtaylor added a commit to robtaylor/abc that referenced this pull request Jun 28, 2026
Incorporate authoritative direction from Alan Mishchenko / Dan Ravenslofty
and the PR threads:

- Record Alan's endorsement of lightweight vOrigins and his explicit engine
  list (which names &nf); add a verified engine-coverage table showing &nf is
  the sole remaining uninstrumented engine.
- Record the acceptance criterion: zero change to default behavior.
- Mark the Nr_Man_t retention-manager lineage (YosysHQ/abc#41, Silimate#4,
  incl. its classic-abc/write_blif path) as REJECTED by Alan; do not revive.
- Note upstream-home reality: yosys consumer (YosysHQ/yosys#5712) closed ->
  fork-only; "abc9 everywhere" ruled out (YosysHQ/yosys#5679 removed abc9
  -liberty). abc side has a home via berkeley-abc#487.

Co-developed-by: Claude Code v2.1.195 (claude-opus-4-8)
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.

4 participants