Reproduce the reference implementation's output

[hansfbaier]

This makes fpga-as produce the same configuration as the chain it replaces on a
database that carries the openXC7 frame annotation work, and fixes the cases
where it produced no bitstream at all. The commit message has the full
write-up; this is the evidence behind it.

Verification

The two chains are not byte-comparable: the .bit header names the tool that
wrote it, and the two writers lay the stream out differently even when the
configuration is identical. So both sides were dumped as frames text --
fasm2frames prints that natively, fpga-as now reproduces it with
--dump_frames_file -- and compared frame by frame, by address, ignoring the
frames each side pads with zeros:

designs	families	result
23	artix7, kintex7, spartan7, zynq7, virtex7	all 23 identical

The set is the 22 designs in openXC7/demo-projects that carry a .fasm (the
blinkies, the LiteX DDR SoCs, the DSP/LUTRAM/DDR3 tests) plus
vc707-johnson from this repository's examples, which forces the HP bank
paths -- differential and single-ended HP IOBs, a LIOB18_SING tile, BUFG.

Wall clock, assembler only, idle machine:

design	fasm2frames + xc7frames2bit	fpga-as
blinky-digilent-arty (artix7)	1.13 s	0.086 s	13x
ddr3-test-arty-s7 (spartan7)	7.05 s	0.36 s	20x
litex-ddr-kc705 (kintex7)	23.4 s	1.33 s	18x
vc707-johnson (virtex7)	48.8 s	0.51 s	96x
blinky-ypcb003381p1 (kintex7)	59.0 s	0.53 s	112x

bazel test //fpga/... passes, 12 tests, three of them added here (a value
range wider than the value over a range wider than 64 bits, and the HP bank
anchor tile naming).

What was wrong

Feature resolution:

• ranges wider than 64 bits reported a chunk width taken from the range, and
  the masking shift then wrapped: RXCDR_CFG[82:0] on the GTP tiles lost
  every value bit above 19, and the reported width above 64 made the caller
  shift past its value and set bits at addresses the design never asked for.
• pseudo pips are documented per tile type, and looking them up in the type a
  tile's bits alias to aborted on tiles such as RIOI_SING.
• a tile type with its own segbits database must not follow its alias; on the
  virtex7 LIOB18_SING tiles that alias is wrong to begin with.
• a feature no database defines now returns an error rather than terminating
  the process through a container .at() (this build has exceptions off).
• the bank anchor is HCLK_IOI on the HP-only parts, not HCLK_IOI3.

Injected configuration: the PUDC_B pullup was injected into every design
instead of being opt-in, the STEPDOWN tag was split on the dots that are part
of it so DDR designs never filled their bank, the HP bank glue / GFAN tie
root / BUFRCLK channel markers were missing, and required_features.fasm was
never read (a zynq7 bitstream missed the processing system's configuration).

Each injected feature is probed before use: they only exist in databases
annotated with them.

Context

This came out of retiring openXC7/nextpnr-xilinx: the toolchain wants one
assembler instead of fasm2frames + xc7frames2bit plus their Python
environment, and fpga-as has been sitting in the devshell unused.

[hzeller]

Nice! I've added a few comments, but I leave the review to @lromor

You mention the comparison between fasm2frames + xc7frames2bit and fpga-as. I think it would be super-cool to have this as a shell script (download the .fasm files at a particular github has upstream, run both toolchains over it and compare), then we can run this as a smoke test in the CI. That way we make sure not regressions are introduced.

Maybe test/smoke-tests.sh ?

[hansfbaier]

Thanks @hzeller — pushed 2368bab addressing the three comments.

Flags to the top (assembler.cc:515): all four ABSL_FLAGs now sit below the includes, comments included.

Raw strings (assembler.cc:517): the flag help strings are regular (adjacent-literal) strings, with \n/\" so the rendered help is unchanged. The only raw string left is the multi-line usage text in Usage().

Platform-specific code (assembler.cc:171): the injected configuration moved to fpga/injected-features.{h,cc} behind InjectConfigurationFeatures(). assembler.cc is 762 → 328 lines: flags, FASM parse, frame pipeline, main(). Also dropped the unused TileGridInfoAndSegbits. If you or @lromor want the file reduced to main() + flags, the remaining pipeline can move to an fpga/assembler.{h,cc} the same way — say so and I will add it.

No behaviour change: same database, same inputs, the pre- and post-split binaries assemble the demo designs to byte-identical frames (20 designs over artix7, kintex7, spartan7 and zynq7, compared frame by frame; the .bit outputs differ only in the header's date/time field). bazel test //fpga/... passes 12/12.

On the smoke test (test/smoke-tests.sh): not in this commit, and I would like your call on the shape before adding it. What it needs to be a CI gate:

• the .fasm corpus at a pinned openXC7/demo-projects revision, downloaded in the job;
• the reference chain in the CI image — xc7frames2bit is packaged here but fasm2frames needs the fasm Python module, and with pip install fasm in the job the corpus above is just a curl/jq away;
• a database with the frame annotation work — which revision should CI pin, and is that database something we can fetch from a public URL in CI, or should the job build it from openXC7/prjxray-db?
• the comparison itself is the one used for this PR: dump both sides as frames text (fasm2frames natively, fpga-as --dump_frames_file), then diff by address ignoring the all-zero frames each side pads with.

Tell me the answers on the last two points (or point me at an existing CI job that does this) and I will write and wire it up.

[lromor]

If you or @lromor want the file reduced to main() + flags, the remaining pipeline can move to an fpga/assembler.{h,cc} the same way — say so and I will add it.

It's ok for now.

On the smoke test (test/smoke-tests.sh): not in this commit, and I would like your call on the shape before adding it. What it needs to be a CI gate:

Sounds good. For now I would prefer to just have the "gold standard" files and ensure we match them. Especially whenever we encounter an edge case that doesn't seem to work as expected.

[hansfbaier]

Reopening isn't available for a merged PR (GitHub only reopens closed-unmerged ones), so here is the material you asked for as a gist:

https://gist.github.com/hansfbaier/20a7b7da75d2bf8fc28ad92936d620e2

It follows your preference — the "gold standard" files, with a pointer at each edge case that didn't behave:

#	case	gold (reference chain)	before the fix	after
01	missing feature, one line	clean error	SIGABRT, core dumped	clean error
02	zynq7 required_features.fasm	2 frames	9 payload bytes differ	== gold
03	PUDC_B pull-up, opt-in	44 frames	7 payload bytes differ	== gold
04	83-bit value, bits 4 and 12	1 frame	7 payload bytes differ	== gold
05	83-bit value, the value from litex_pcie.fasm	2 frames	—	differs from gold in 2 frames — open

Case 05 is the one that still does not match: the design's own RXCDR_CFG[82:0] value, assembled in isolation, makes the fixed assembler disagree with fasm2frames in frames 0x0002129e and 0x0002129f — 18 bits present only in the reference, 5 only in fpga-as; the bit positions are in the gist. I have not identified the cause yet.

Each case carries: the smallest input (.fasm), the reference's frames (gold, canonicalised to the frames that are non-zero), the fixed assembler's frames, the failed and fixed bitstreams (gzipped and base64-wrapped, since a gist cannot store binary — base64 -d X.bit.gz.b64 | gunzip > X.bit), the payload byte diff between the two builds, and the stderr of all three. The harness that produced them is there too, plus corpus_results.tsv, the 22-design demo-projects sweep.

Method note: the pre-fix build has no --dump_frames_file (the option came with #47), so its evidence is the .bit it wrote plus a payload diff against the fixed build — same writer, so the layouts line up. Everything else is compared frame by frame over the set of configuration bits.

On the repository state: main's tip is still e394738 and fpga/injected-features.cc is present, i.e. the revert in #48 is not in effect and nothing from #47 was lost. If you want the change back through review, I can open a fresh PR from hansfbaier:reference-parity with these files added under fpga/xilinx/testdata/ — next to xc7-configuration.bit, where a test can check fpga-as against them (skipped without a database, since the db is not committed). Say which you would prefer, or take the gold files from the gist as they are.

[lromor]

Amazing, thank you @hansfbaier !

[hansfbaier]

Two additions to the gist, now that the corpus sweep has finished.

The sweep is complete (corpus_results.tsv, all 21 demo-projects designs that carry a .fasm):

• before the fix: 19 produced a different configuration than the fixed build, and 2 terminated without writing a bitstream at all (litex-ddr-kc705, and litex-sata-alientek-davincipro — case 01's full design);
• after the fix: frame-identical to the reference chain on all 20 designs the reference can assemble. The 21st is the database gap of case 01, which the reference cannot assemble either.

New case 06 narrows the litex-ddr-kc705 abort to a single line:

RIOI_SING_X95Y100.IOI_LOGIC_OUTS18_0.IOI_ILOGIC0_O

It is a pseudo PIP: the reference and the fixed assembler both accept it, and it maps to no configuration bits at all — the frames agree (and are empty). Before the fix that same line dies in raw_hash_map::at, because the pseudo PIP is looked up in the database of the tile type this tile's bits alias to, which does not document it. On the whole design that meant no bitstream at all; the eight RIOI_SING lines of the design are in the gist too, and the fix assembles them to the reference's frames exactly.