regression: guard the two IOB divergences from Vivado, and add reject.txt - #15
regression: guard the two IOB divergences from Vivado, and add reject.txt#15gHashTag wants to merge 2 commits into
Conversation
….txt Adds iob-lvcmos33-drive-slew, guarding openXC7/nextpnr-xilinx#120: an LVCMOS33 output at the default drive was getting the I12_I8 bit pattern where Vivado emits I12_I16, and every input-only pad was getting output SLEW.SLOW bits. This is the first case here whose criterion is not "the flow used to fall over". Both bugs produced a valid bitstream that merely programmed the pad differently, so the expected values are transcribed from the four Vivado-built bitstreams prjxray-db already ships in artix7/harness/, not from our own output. Their design.dcp files carry Vivado's top_late.xdc, which constrains nothing but PACKAGE_PIN and IOSTANDARD LVCMOS33, so the comparison is like-for-like. It also needs a check the runner did not have. A fix that makes us stop emitting a wrong bit is invisible to expect.txt, because the absence of a bit is not a string you can grep for. So cases may now carry a reject.txt, same format, and fail if any pattern matches. This case uses both: expect.txt for the drive pattern that should now appear on the output pad, reject.txt for the wrong pattern and for the slew bits that should no longer appear on the input pad. Checked against unpatched nextpnr-xilinx, where it fails with both divergences reproduced, so it is a real guard rather than a tautology. Suite on 0.9.2 + the #120 patch, chipdb xc7a200tfbg484-2: clock-srcc-bufg ok bram-sdp-unused-port ok bufg-fabric-driven ok config-primitive-startupe2 ok iddr-four-iff-flops ok iob-lvcmos33-drive-slew ok
The suite was moved here from nextpnr-xilinx in openXC7/nextpnr-xilinx#118 because this repo has the chipdb and working CI -- but nothing was ever wired up to invoke it. `grep -rn "run.sh" .github/workflows/` returns nothing, so six cases guarding six fixed bugs have been running nowhere, and the move did not achieve what it was for. Adds a `regression` job that reuses the chipdb-artix7 artifact stage 1 already produces. The cases need only yosys, nextpnr-xilinx and one chipdb, and they stop at the FASM, so this costs minutes rather than the hours the demo matrix takes. The job asserts the chipdb is present before running. Without that check an absent artifact would fail every case for the wrong reason, which on the job list looks exactly like a real regression -- the same indistinguishable-failure problem .DELETE_ON_ERROR was added for in openXC7#13. Note this runs the pinned toolchain's nextpnr-xilinx, which is correct here: this repo tests the released flow. The equivalent gate on the nextpnr-xilinx side has the opposite requirement and is currently not meeting it -- see openXC7/nextpnr-xilinx#125.
|
Two updates, plus a note on the red checks. The red checks are not this PR
and the failing jobs are So: pre-existing, and worth a separate look at the The suite was not being run at allWhile checking whether my new case would actually be exercised, I found it would not: The suite was moved here from nextpnr-xilinx in nextpnr-xilinx#118 precisely because this repo has the chipdb and working CI — but nothing was ever wired up to invoke it. Six cases guarding six fixed bugs have been executing nowhere since the move, so the move has not yet done what it was for. ("regression" does appear in Second commit adds a It also asserts the chipdb is present before running, because otherwise an absent artifact fails every case for the wrong reason, and on the job list that is indistinguishable from a real regression — the same problem Happy to split that second commit into its own PR if you would rather keep this one to the single case; it seemed more useful attached to the thing that revealed it. Note on which binary the suite testsThis job runs the pinned toolchain's nextpnr-xilinx, which is right for this repo — it tests the released flow. The gate on the nextpnr-xilinx side needs the opposite and currently is not getting it; see nextpnr-xilinx#125. |
|
@gHashTag Sorry, I somehow overlooked this. I just pushed somewhat significant changes, can you check if there are conflicts with the latest changes? |
2 participants
Pairs with nextpnr-xilinx#120 — draft for the same reason it is: that patch changes the bitstream of every design with an LVCMOS33 pad, so the goldens in this repo need regenerating before either lands. This PR does not touch them.
The case
iob-lvcmos33-drive-slewguards two divergences from Vivado that produced a perfectly valid bitstream and merely programmed the pad differently:LVCMOS33_LVTTL.DRIVE.I12_I8where Vivado emitsI12_I16— two bits, on every output pad;SLEW.SLOWbits. On the arty-a7 pinout that was 17 pads where Vivado sets 8.Because both produced a valid bitstream, this is the first case here whose criterion is not "the flow used to fall over", and the expected values had to come from somewhere other than our own output.
Where the expectations come from
prjxray-dbalready ships four Vivado-built bitstreams with their design checkpoints, and nothing seems to use them as a reference:prjxray/utils/bit2fasm.pyturns them back into FASM, and each.dcpcontains Vivado's owntop_late.xdcconstraining nothing butPACKAGE_PINandIOSTANDARD LVCMOS33— so every pad is at Vivado's defaults and the comparison is like-for-like. Across all four, 35 output pads, the drive pattern isI12_I16every time andI12_I8never;SLEW.SLOWcount equals the output-pad count every time.reject.txtThe runner needed a check it did not have. A fix that makes us stop emitting a wrong bit is invisible to
expect.txt, because the absence of a bit is not a string you can grep for — the case would have passed before the fix too.So a case may now carry a
reject.txt, same one-regex-per-line format, and fails if any pattern matches. This case uses both:expect.txtfor the drive pattern that should now appear on the output pad,reject.txtfor the wrong pattern and for the slew bits that should no longer appear on the input pad.Checked
Against unpatched nextpnr-xilinx it fails, with both divergences reproduced in the FASM:
so it is a real guard and not a tautology. With #120 applied, the full suite on chipdb
xc7a200tfbg484-2:The README gains a section on the two artefact classes that have to be filtered out of any FASM-vs-disassembled-FASM diff (all-negated features and
alwayspseudo-pips) — in the arty-a7/swbut comparison they accounted for 84 and 42 lines, and reading them as real differences is the easy mistake here.