From 743969610890d5546f4b06f3023f190134fbd720 Mon Sep 17 00:00:00 2001 From: Jason Zeng Date: Thu, 20 Aug 2026 11:20:19 -0400 Subject: [PATCH 1/2] xilinx: DSP48E1 AREG_2_ACASCREG_1 is a conjunction, not ACASCREG 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 --- xilinx/fasm.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/xilinx/fasm.cc b/xilinx/fasm.cc index fc7d7a61..cec5fbba 100644 --- a/xilinx/fasm.cc +++ b/xilinx/fasm.cc @@ -5176,8 +5176,20 @@ void write_gtx_channel(CellInfo *ci) write_bit("ZADREG[0]", !bool_or_default(ci->params, ctx->id("ADREG"), true)); write_bit("ZALUMODEREG[0]", !bool_or_default(ci->params, ctx->id("ALUMODEREG"))); - write_bit("ZAREG_2_ACASCREG_1", !bool_or_default(ci->params, ctx->id("ACASCREG"))); - write_bit("ZBREG_2_BCASCREG_1", !bool_or_default(ci->params, ctx->id("BCASCREG"))); + // AREG_2_ACASCREG_1 is prjxray's name for a CONJUNCTION, not for + // ACASCREG: fuzzers/100-dsp-mskpat/generate.py sets it to + // (AREG == 2 && ACASCREG == 1). Its Z-form bit therefore belongs in + // the bitstream whenever that conjunction is FALSE. Deciding it from + // ACASCREG alone mis-encodes the case yosys emits whenever it absorbs + // one level of input register into the DSP -- AREG=1 with + // ACASCREG=1 -- which wrote no bit at all and reads back on silicon + // as AREG=2, one pipeline stage deeper than the netlist asked for. + // The multiply then samples its operands a cycle late and the design + // returns wrong products with a correct netlist. Same for B. + auto acascreg = int_or_default(ci->params, ctx->id("ACASCREG"), 1); + auto bcascreg = int_or_default(ci->params, ctx->id("BCASCREG"), 1); + write_bit("ZAREG_2_ACASCREG_1", !(areg == 2 && acascreg == 1)); + write_bit("ZBREG_2_BCASCREG_1", !(breg == 2 && bcascreg == 1)); write_bit("ZCARRYINREG[0]", !bool_or_default(ci->params, ctx->id("CARRYINREG"))); write_bit("ZCARRYINSELREG[0]", !bool_or_default(ci->params, ctx->id("CARRYINSELREG"))); write_bit("ZCREG[0]", !bool_or_default(ci->params, ctx->id("CREG"), true)); From a204ae52f15e8b1aee4b2bea83e0ccc4cf1b85b4 Mon Sep 17 00:00:00 2001 From: Jason Zeng Date: Thu, 20 Aug 2026 11:21:15 -0400 Subject: [PATCH 2/2] xilinx: give DSP48E1 INMODE/ALUMODE2/3/OPMODE6 their tile constant bits 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__.DSP_GND_[LR] DSP_[LR].DSP__.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 --- xilinx/fasm.cc | 35 +++++++++++++++++++++++++++++++++-- xilinx/pack_dsp_xc7.cc | 29 +++++++++++++++++++++++++---- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/xilinx/fasm.cc b/xilinx/fasm.cc index cec5fbba..e40cbd08 100644 --- a/xilinx/fasm.cc +++ b/xilinx/fasm.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "log.h" #include "nextpnr.h" @@ -5213,9 +5214,39 @@ void write_gtx_channel(CellInfo *ci) boost::split(pins, attr_value, boost::is_any_of(" ")); for (auto pin : pins) { if (boost::empty(pin)) continue; + // These pins have no interconnect path into the site, so the + // tile bit IS the pin: it bypasses the site's optional input + // inverter, and the ZIS_*_INVERTED bits fasm.cc writes apply + // only to the routed pins. The bit therefore has to carry the + // LOGICAL value -- flip the constant when the netlist asked + // for an inversion. + // + // Splitting the trailing index off matters: erase_all() of + // "0123456789" turned "INMODE1" into "INMODE" and then looked + // up IS_INMODE_INVERTED, which yosys never emits (it writes + // the per-bit IS_INMODE[1]_INVERTED), so `inv` was always + // false and every bussed pin got the un-flipped constant. + // That is the "seems to be inverted for unknown reasons" that + // kept INMODE/ALUMODE2/ALUMODE3 commented out of the packer's + // const-pin list in pack_dsp_xc7.cc. auto pin_basename = pin; - boost::erase_all(pin_basename, "0123456789"); - auto inv = bool_or_default(ci->params, ctx->id("IS_" + pin_basename + "_INVERTED"), 0); + std::string idx; + while (!pin_basename.empty() && std::isdigit((unsigned char)pin_basename.back())) { + idx.insert(idx.begin(), pin_basename.back()); + pin_basename.pop_back(); + } + bool inv = bool_or_default(ci->params, ctx->id("IS_" + pin + "_INVERTED"), false); + if (!idx.empty()) { + inv |= bool_or_default( + ci->params, + ctx->id("IS_" + pin_basename + "[" + idx + "]_INVERTED"), false); + inv |= ((int_or_default(ci->params, + ctx->id("IS_" + pin_basename + "_INVERTED"), 0) + >> std::stoi(idx)) & 0x1) != 0; + } else { + inv |= bool_or_default(ci->params, ctx->id("IS_" + pin_basename + "_INVERTED"), + false); + } auto net_name = inv ? (const_net_name == "GND" ? "VCC" : "GND") : const_net_name; write_bit(dsp + "_" + pin + ".DSP_" + net_name + "_" + tile_side); } diff --git a/xilinx/pack_dsp_xc7.cc b/xilinx/pack_dsp_xc7.cc index c78250a1..4e896126 100644 --- a/xilinx/pack_dsp_xc7.cc +++ b/xilinx/pack_dsp_xc7.cc @@ -121,12 +121,33 @@ void XC7Packer::pack_dsps() // prjxray has extra bits for these ports to hardwire them to VCC/GND // as these seem to be internal to the tile, // this saves us from having to route those externally + // The pins below have NO routing path into the site at all: + // they appear in prjxray's segbits_dsp_{l,r}.db only as + // .DSP_GND_* / .DSP_VCC_* and in no ppips/pips + // list, so a tile-local constant bit is the ONLY way to give + // them a value. Leaving one out does not fail routing -- it + // silently emits no bit, the pin sits at the tile default 0, + // and the site's ZIS_*_INVERTED bit (written independently by + // fasm.cc's write_bus_zinv) flips it to 1. + // + // INMODE, ALUMODE2/3 and OPMODE6 were missing. For a plain + // inferred multiplier that turns INMODE 00000 into 11111 -- + // and INMODE[1]=1 gates the multiplier's A input to zero + // (UG479 Table 1-11) -- OPMODE[6:4] 000 into 100 (P feedback + // instead of zero) and ALUMODE[3:2] 00 into 11. The DSP then + // ignores its A operand entirely and returns near-constant + // junk, with a correct netlist and clean timing. + // + // The earlier "these seem to be inverted for unknown reasons" + // is that ZIS_*_INVERTED: the pin must be driven with the + // UNINVERTED value (VCC for a wanted 0 when the site inverts), + // which is what connecting it to its actual constant net does. if (boost::starts_with(n, "D") || boost::starts_with(n, "RSTD") || - // TODO: these seem to be inverted for unknown reasons - // boost::starts_with(n, "INMODE") || - // boost::starts_with(n, "ALUMODE2") || - // boost::starts_with(n, "ALUMODE3") || + boost::starts_with(n, "INMODE") || + boost::starts_with(n, "ALUMODE2") || + boost::starts_with(n, "ALUMODE3") || + boost::starts_with(n, "OPMODE6") || boost::starts_with(n, "CARRYINSEL2") || boost::starts_with(n, "CED") || boost::starts_with(n, "CEAD") ||