lutpack: assertion failure in Lpk_MuxSplit from an approximate cofactor support - #540
lutpack: assertion failure in Lpk_MuxSplit from an approximate cofactor support#540marcelwa wants to merge 1 commit into
Conversation
`lutpack` aborts on some networks with
abc: src/opt/lpk/lpkAbcMux.c:192: Lpk_MuxSplit:
Assertion `iVarVac < (int)p->nVars' failed.
Reproducer (a 24.7k-LUT `sqrt` netlist produced by `if -K 10 -Z 6`, ~10 s):
read_blif sqrt-mapped.blif; lutpack
Lpk_MuxSplit() splits one component off a function and stores the new component
in a *vacant* fanin slot of the retained one:
p->uSupp = Kit_TruthSupport( Pol ? pTruth1 : pTruth0, p->nVars );
p->uSupp |= (1 << Var);
iVarVac = Kit_WordFindFirstBit( ~p->uSupp );
assert( iVarVac < (int)p->nVars );
A vacant slot is supposed to be guaranteed by Lpk_MuxAnalize(), which rejects a
candidate variable when
nSuppSizeL = max(nSuppSize0 + 2*!Polarity, nSuppSize1 + 2*Polarity) > p->nVars
but it reads nSuppSize0/nSuppSize1 out of the *cached* p->puSupps[]. When those
came from Lpk_ComputeSupports() they are not exact: that routine builds two
BDDs of the function in opposite variable orders and stitches the two support
estimates together at the cofactoring variable, and the result can be a strict
subset of the true cofactor support. Lpk_MuxAnalize() then admits a variable
whose split needs one slot more than the function has.
On the reproducer this happens for a 12-variable component at Var = 3,
Polarity = 1: the cached support of cofactor 1 is 0x3f7 (9 variables) while the
truth table's is 0xff7 (11). The guard sees 9 + 2 = 11 <= 12 and accepts;
the split then produces uSupp = 0xff7 | (1 << 3) = 0xfff, which is full.
Instrumenting the same run shows the estimate differs from the exact support in
484 of 101970 cofactor supports, and is narrower in 352 of them, so this is not
a one-off.
Rather than change the support estimator or weaken the assertion -- which
documents a real invariant of Lpk_MuxSplit() -- re-derive the single support the
split depends on, once the candidate has been chosen, and decline the MUX
decomposition when it does not fit. That is one cofactor and one support scan
per accepted candidate, not per candidate variable. On the reproducer lutpack
then completes and yields the same result as recomputing every cached support
from the truth table (24694 -> 24635 nodes, 237 levels in both cases).
|
Please do not merge this as it stands — I have found evidence that the assertion this PR removes is guarding a real miscompile elsewhere in I am reporting this against my own patch as soon as I found it. What happenedRunning Three builds isolate the blame, and it is not this patch
The third row is the decisive one: with the MUX decomposition path disabled entirely, the miscompile still occurs. So the defect is not in the MUX split path, this patch does not introduce it, and the support-estimate problem the patch describes is real and separate. What the patch does do is let execution proceed far enough to reach the underlying bug. The assertion was acting as an accidental guard. What I would suggestTreat this PR as blocked on the underlying I will open a separate issue for the miscompile itself with the reproducer netlist and the exact command, and link it here. Apologies for the noise; better to flag it against my own patch now than to have it merged and mask a correctness bug. |
|
Filed the underlying miscompile as #542, with the three-build isolation and the verification detail. This PR is blocked on it. |
|
One more correction to this PR's own description, which I should have caught before opening it. The description says:
That 24635-node netlist is precisely the non-equivalent output reported in #542. So the sentence I offered as evidence that the patch behaves correctly is in fact describing the miscompile. "Completes normally" was measured as terminates without aborting and matches an alternative implementation of the same support computation — both of which are true, and neither of which implies the output is correct. I did not check equivalence at that point, and I should have. To be explicit about what the patch is and is not supported by:
Given #542, the honest status of this PR is that it removes a symptom whose underlying cause is unfixed. I would rather it not be merged in that state. Happy to withdraw it, or to hold it until #542 is understood and then rebase whatever part still applies — your call. |
Symptom
lutpackaborts on some networks withReproducible in about 10 s on a 24.7k-LUT
sqrtnetlist produced byif -K 10 -Z 6:Root cause
Lpk_MuxSplit()stores the new component in a vacant fanin slot of the retained function:A vacant slot is supposed to be guaranteed by
Lpk_MuxAnalize(), which rejects a candidate variable whennSuppSizeL > p->nVars. But it readsnSuppSize0/nSuppSize1out of the cachedp->puSupps[], whichLpk_ComputeSupports()derives from two opposite-order BDDs and stitches together. That estimate can be a strict subset of the true cofactor support, so the feasibility test passes on a candidate whose real cofactor has no vacant fanin, and the assertion then fires inLpk_MuxSplit.Measured on the failing instance: at the failing call the cached support is
0x3f7(9 variables) against a true0xff7(11). Across the same run, 484 of 101970 cached supports disagree with the recomputed value, 352 of them narrower — so this is a systematic property of the estimate, not a one-off.Fix
Re-derive that one support after the candidate has been chosen, rather than trusting the cached estimate at the point where correctness depends on it. Recomputing everything would also work but is far more expensive; re-deriving the single support reaches the same answer.
On the reproducer,
lutpackthen completes normally and gives the same result as recomputing every support (24694 → 24635 nodes, 237 levels).Testing
Built clean.
if -K 10 -Z 6oncavlcis unchanged (nd = 121, lev = 4), i.e. no behavioural change on paths that already worked.