xilinx: inferred DSP48E1 ignores its A operand -- INMODE/ALUMODE2/3/OPMODE6 never get their tile constant bits - #159
jasonzeng124 wants to merge 2 commits into
Conversation
prjxray defines the DSP48E1 feature AREG_2_ACASCREG_1 as (AREG == 2 && ACASCREG == 1) -- see fuzzers/100-dsp-mskpat/generate.py. fasm.cc derived its Z-form bit from ACASCREG alone, ignoring AREG, so the configuration yosys emits whenever synth_xilinx absorbs one level of input register into the multiplier (AREG=1, ACASCREG=1) wrote no bit at all. Silicon reads that back as AREG=2: one input pipeline stage more than the netlist asked for. Same for BREG/BCASCREG. Found while bisecting wrong products out of an inferred DSP48E1 on xc7z010clg400 (EBAZ4205). This one is a latent mis-encoding rather than the cause of those wrong products -- the design under test read its result milliseconds later, so an extra input pipeline stage was invisible to it and the board result was bit-identical before and after. The real cause is the following commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Some DSP48E1 site pins have no interconnect path into the site at all.
In prjxray they appear in segbits_dsp_{l,r}.db only as
DSP_[LR].DSP_<n>_<PIN>.DSP_GND_[LR]
DSP_[LR].DSP_<n>_<PIN>.DSP_VCC_[LR]
and in no ppips/pips list, so a tile-local constant bit is the ONLY way to
give them a value. For xc7z010 (and every other 7-series part in the db)
that set is
D0..D24 RSTD CARRYINSEL2 CED CEAD CEINMODE CEALUMODE
INMODE0..4 ALUMODE2 ALUMODE3 OPMODE6
pack_dsps() converted the first seven groups and left the last three out.
Those pins therefore stayed attached to $PACKER_VCC_NET / $PACKER_GND_NET
and were handed to the router, which has nowhere to take them. That is not
a routing failure -- nothing errors, nothing warns, no bit is emitted, and
on silicon the pin comes up as the complement of what the netlist asked
for.
For a plain inferred `a * b` the netlist wants INMODE=00000, OPMODE=0000101
and ALUMODE=0000. Without the bits the part sees INMODE=11111,
OPMODE=1000101 and ALUMODE[3:2]=11. INMODE[1]=1 gates the A operand of the
multiplier to zero (UG479 Table 1-11) and OPMODE[6:4]=100 feeds P back into
the Z mux instead of zero, so the DSP ignores A entirely and returns
near-constant junk -- with a correct netlist, a correct post-synthesis
simulation and clean timing. Nothing before the bitstream shows it.
The second half of the fix is in fasm.cc. These bits bypass the site's
optional input inverter -- the ZIS_*_INVERTED bits apply only to the routed
pins -- so the constant chosen has to be the LOGICAL value.
write_const_pins() already tried to do that, but it stripped the digits off
the pin name before looking the parameter up, asking for
IS_INMODE_INVERTED when yosys emits the per-bit IS_INMODE[1]_INVERTED. The
lookup always returned false, so every bussed pin got the un-flipped
constant. That is almost certainly the "TODO: these seem to be inverted
for unknown reasons" that kept INMODE and ALUMODE2/3 commented out here in
the first place: adding them without fixing the lookup ties them to VCC and
makes the DSP worse, not better.
Measured on an EBAZ4205 (xc7z010clg400), yosys 0.5x + this nextpnr, reading
results back over JTAG. A 16x16 multiply whose entire datapath is one `*`:
before, walking all 16 bits of A changed the output not at all (one lane
returned 0 for every bit, another a constant 0x18000000) and walking B gave
wrong values; after, all 16 A bits and all 16 B bits are exact. Full runs,
before -> after:
two lone 16x16 DSPs, 415 random vectors : 1/315, 315/315 -> 415/415, 415/415
32x32 via a 3-DSP cascade, 430 vectors : 25/430 -> 430/430
a real compiled kernel using a multiply : 965/2016 -> 516/516
The failure was deterministic and clock-independent: dropping FCLK0 from
100 MHz to 10 MHz reproduced it bit-for-bit, which is what ruled out timing
and pointed at the bitstream.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Can you supply a design to reproduce as a unit test? Gist would be fine. |
|
https://gist.github.com/jasonzeng124/25677dfa5b51533036b9a8b79ab5f38a here you go, i'm not sure if it really makes sense as a unit test without running it on hardware though. |
hansfbaier
left a comment
hansfbaier
left a comment
There was a problem hiding this comment.
Two-axis review (Standards / Spec)
Reviewed a204ae52 vs base 68aeeb39 (three-dot): 2 commits, 2 files (xilinx/fasm.cc, xilinx/pack_dsp_xc7.cc), +72/−8. Spec = the PR body; standards = docs/coding.md (+ .clang-format, not CI-enforced).
Standards
No violations of docs/coding.md (it's a philosophy doc, no rule broken here). Both hunks use the existing helpers exactly as neighbours do; acascreg/bcascreg mirror areg/breg at fasm.cc:5120/5129. Judgement calls:
- Duplicated Code — the new inversion lookup in
write_const_pinsrepeats the shape ofwrite_bus_zinv(~145 lines above, same function):(int_or_default("IS_"+name+"_INVERTED") >> i) & 0x1OR'd with the per-bit form. Two copies of "resolve a per-bit inversion param" now live in one file; consider onepin_inverted(name, bit)helper used by both. - Dead branch — when
idxis empty,pin_basename == pin, so theelsere-reads the identical IdString already read intoinv; the OR can never changeinvforRSTD/CED/CEAD/CEINMODE/CEALUMODE. - Speculative Generality — three parameter spellings per pin are accepted, while the new comment itself says yosys only emits the per-bit
IS_INMODE[1]_INVERTEDform; theIS_INMODE1_INVERTEDlookup covers a form the change asserts nobody writes. - Primitive Obsession / Data Clumps —
std::string idxcarries a bit index as text through all three lookups and is converted back with an unguardedstd::stoi(idx);(basename, bit)wants to be a small type. - IdString churn (soft, consistent with pre-existing code) — up to 3 constructed names interned per const pin per DSP, most matching no parameter.
- Shotgun Surgery — one logical change ("these pins get a tile-local constant bit") required edits in both files; the pin set lives in a
boost::starts_withcascade (now 9 predicates) instead of one table both sites read. - Comment-to-code ratio — 43 of 72 added lines are comment; the pack hunk adds 21 comment lines for 4 code lines. The UG479/prjxray citations are worth keeping; the defect narrative belongs in the commit message.
Spec
(a) Missing/partial: none. All three spec'd changes are present. No automated test, but the spec only claims hardware testing.
(b) Scope creep: commit 1 (ZAREG_2_ACASCREG_1 / ZBREG_2_BCASCREG_1 conjunction) is unrelated to the PR title — the body itself says "latent mis-encoding rather than the cause above" and offers to split it out.
(c) Overclaimed / wrong:
- "The only difference in the emitted FASM is eight added
DSP_<n>_<PIN>.DSP_GND_*lines per DSP" is false in general. ForAREG=1 && ACASCREG=1(the register-absorption config the PR sayssynth_xilinxemits), the old!bool_or_default(ACASCREG)emitted no line, while the new!(areg==2 && acascreg==1)emits aZAREG_2_ACASCREG_1line (plusZBREG_2_BCASCREG_1for B) — up to two extra lines beyond the eight. The "eight" holds only for the plain-multiply tests. - Undisclosed default flip:
ACASCREG/BCASCREGdefault changed 0 → 1 (bool_or_default(..., ACASCREG)vsint_or_default(..., ACASCREG, 1)). WithAREG=2and the parameter absent, old emittedZAREG(bit 0), new emits nothing (bit 1). Likely benign, but worth a line in the description.
Verified correct (attempted falsification against prjxray-db, all failed):
- The const-pin set is exactly complete vs
zynq7/segbits_dsp_l.db(D0–24, RSTD, CARRYINSEL2, CED, CEAD, CEINMODE, CEALUMODE, INMODE0–4, ALUMODE2/3, OPMODE6); no GND/VCC entry exists outside the packer list. ppips_dsp_l.dbhas ALUMODE0/1 and OPMODE0–5 but no ALUMODE2/3, OPMODE6 or INMODE0–4 (the only "INMODE" match isRSTINMODE, a different port) — the "no routing path" claim holds.AREG_2_ACASCREG_1is defined as(AREG == 2 && ACASCREG == 1)infuzzers/100-dsp-mskpat/generate.py; the db has no standalone ACASCREG bit.areg/bregare in scope at the newwrite_bitcalls; the bitmask shift mirrorswrite_bus_zinvandidx ≤ 24keeps it in range.
Bottom line
Standards: 0 hard violations, 7 judgement-call smells — worst is the inversion-resolution logic duplicated across the two fasm.cc lambdas plus the pin-set knowledge split across both files. Spec: 3 findings — worst is the overstated FASM-delta claim, alongside the undisclosed ACASCREG/BCASCREG default flip. The fix itself looks technically sound and is hardware-validated; splitting commit 1 out (as the author offered) would make the history cleaner.
|
@jasonzeng124 I tested the reproducer design on Spartan7 and could not reproduce the issue on hardware
Hi I created a design for a hardware board, that output the DSP results over serial. Can you adapt and test this on your board: |
|
Hi, unfortunately I'm not around my board for a while. I do remember it was a zynq-7010 on a ebaz4205 board though. |
#158 (LUT6_2 X_ORIG_PORT) is fixed and merged into main (7cfd1e9), and toolchain-nix is pinned to 4f78e46 (which pins nextpnr 7cfd1e9), so lut_shared_pin is green again and back in the regression default suite. dsp-const-only-pins stays disabled as expected-red until #159 lands. The gate checks out toolchain-nix and demo-projects at their default branches (main), currently 4f78e46 and 72e1ced respectively, so no explicit rev pin needs bumping here.
…in flight The capacity table's load-bearing caveat said the DSP48E1 assumption was unverified, quoting the 'Clocked DSP48E1s are currently unsupported' error. Checked against the toolchain on 2026-08-31: that error was gatecat's 2020 placeholder and the openXC7 fork removed it on 2023-03-07 with real FASM generation (24113d1); as of 0.9.3 the full chain infers, places, routes and bitstreams clocked and cascaded DSP48E1, and the Kintex-7 segbits are in prjxray-db. What remains is openXC7/nextpnr-xilinx#159 (const-only control pins, wrong products on Zynq-7010, fix unmerged), an ~1.5x DSP-count penalty from Yosys's 18x18 partial decomposition that flips no verdict, and timing that ignores registered DSPs. The paragraph now says so, with dates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A constant sink whose site wire has no uphill pip (the DSP48E1 INMODE, ALUMODE and OPMODE pins of openXC7#159) is unreachable for any net. Giving it a driver LUT only made the re-route fail hard inside the router, before --allow-const-holdouts was consulted, so the flag never covered that class. Send such sinks straight to the report, with their own reason, and narrow the help text: a driver LUT net the router cannot route is still a router error.
A constant sink whose site wire has no uphill pip (the DSP48E1 INMODE, ALUMODE and OPMODE pins of openXC7#159) is unreachable for any net. Giving it a driver LUT only made the re-route fail hard inside the router, before --allow-const-holdouts was consulted, so the flag never covered that class. Send such sinks straight to the report, with their own reason, and narrow the help text: a driver LUT net the router cannot route is still a router error.
A constant sink whose site wire has no uphill pip (the DSP48E1 INMODE, ALUMODE and OPMODE pins of openXC7#159) is unreachable for any net. Giving it a driver LUT only made the re-route fail hard inside the router, before --allow-const-holdouts was consulted, so the flag never covered that class. Send these sinks to the report and clarify the help text.
A constant sink whose site wire has no uphill pip (the DSP48E1 INMODE, ALUMODE and OPMODE pins of #159) is unreachable for any net. Giving it a driver LUT only made the re-route fail hard inside the router, before --allow-const-holdouts was consulted, so the flag never covered that class. Send these sinks to the report and clarify the help text.
|
Independent check of this PR against current 1. The packer/fasm gap is realRebased your two commits onto
The added features are exactly 2. One correction to the mechanism
That is true of the fuzz-derived files (no segbit and no ppip for e.g. so "no bit is emitted" isn't accurate for 3. Hardware: no behavioural difference on xc7s50Arty S7-50,
The B and A2 captures are byte-identical. So on this part the plain-multiply case cannot distinguish the two, which is consistent with @hansfbaier's "could not reproduce on Spartan7" — but the design is also insensitive to the bit that changed, see below. 4. Your commit 1 is the half that is distinguishable — and it needs a different designFor the "one 16x16 multiply" case yosys emits
To settle it on hardware without JTAG readback of the DSP, I built a self-checking design: registered-input multiply (
On xc7s50 that A/B will read: control MISMATCH, main ?, PR ?. Whatever it says, it is one flash and one second of UART per phase, and it is the only hardware evidence I can produce for commit 1 with the board I have (an Arty S7-50; the only JTAG idcode here is What I'd ask for
I have the FASM deltas, the four bitstreams, the simulators' output and the board runner ready; happy to hand any of it over, or to open it as a draft PR against your branch. I have not pushed anything. |
2 participants
Summary
A design whose entire datapath is
assign p = a * b;returns wrong products on hardware when the multiply is inferred into a DSP48E1. The netlist is correct, post-synthesis simulation of that netlist passes, and timing is clean — the fault is introduced in the FASM writer, so nothing before the bitstream shows it.Measured on an EBAZ4205 (xc7z010clg400), reading results back over JTAG:
Bit-walk before the fix: sweeping all 16 bits of
AwithB=1changed the output not at all — one lane returned0for every bit, the other a constant0x18000000. SweepingBmoved the output but to wrong values. After the fix, all 16 A bits and all 16 B bits are exact on both DSPs.The failure was deterministic and clock-independent — dropping FCLK0 from 100 MHz to 10 MHz reproduced it bit-for-bit — which is what ruled out timing and pointed at the bitstream.
Cause
Some DSP48E1 site pins have no interconnect path into the site at all. In prjxray they appear in
segbits_dsp_{l,r}.dbonly asDSP_<n>_<PIN>.DSP_GND_*/DSP_<n>_<PIN>.DSP_VCC_*, and in no ppips/pips list, so a tile-local constant bit is the only way to give them a value. That set is:pack_dsps()converted the first seven groups and leftINMODE,ALUMODE2/3andOPMODE6out. Those pins stayed on\$PACKER_VCC_NET/\$PACKER_GND_NETand were handed to the router, which has nowhere to take them. Nothing errors, nothing warns, no bit is emitted — and on silicon the pin comes up as the complement of what the netlist asked for.For a plain inferred multiply the netlist wants
INMODE=00000,OPMODE=0000101,ALUMODE=0000. The part instead seesINMODE=11111,OPMODE=1000101,ALUMODE[3:2]=11:INMODE[1]=1gates the multiplier's A operand to zero (UG479 Table 1-11) — that is the "A does nothing" symptom exactly.OPMODE[6:4]=100feedsPback into the Z mux instead of zero, a combinational loop through the adder, hence the near-constant output.ALUMODE[3:2]=11changes the ALU function on top of that.The second half, in
fasm.ccThese tile bits bypass the site's optional input inverter — the
ZIS_*_INVERTEDbits apply only to the routed pins — so the constant chosen has to be the logical value.write_const_pins()already tried to do that, but it stripped the digits off the pin name before the parameter lookup, asking forIS_INMODE_INVERTEDwhen yosys emits the per-bitIS_INMODE[1]_INVERTED. The lookup always returned false, so every bussed pin got the un-flipped constant.That is almost certainly the
// TODO: these seem to be inverted for unknown reasonsthat keptINMODE/ALUMODE2/ALUMODE3commented out of the packer's list in the first place — I can confirm it experimentally: adding the pins without fixing the lookup ties them to VCC and makes the DSP strictly worse (fully inert, constant0x08000000on every vector). Both halves are needed.Commits
AREG_2_ACASCREG_1is a conjunction, notACASCREG— prjxray defines the feature as(AREG == 2 && ACASCREG == 1)(fuzzers/100-dsp-mskpat/generate.py), but the Z-form bit was derived fromACASCREGalone, so theAREG=1, ACASCREG=1configurationsynth_xilinxemits when it absorbs an input register wrote no bit and reads back asAREG=2. Found on the way; a latent mis-encoding rather than the cause above — the board result was bit-identical before and after it. Happy to split this out if you would rather review it separately.fasm.cclookup.Testing
Built with the openXC7 toolchain and run on real hardware (EBAZ4205 / xc7z010clg400, Zynq PS driving the PL over AXI, results read back over JTAG). Numbers in the table above. No change in behaviour for designs without a DSP48E1; for DSP designs the only difference in the emitted FASM is eight added
DSP_<n>_<PIN>.DSP_GND_*lines per DSP.