luke
/
verilator
mirror of https://github.com/verilator/verilator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix partial DFG conversion of concat assignments (#6255) 

When we had a `{a, b} = ...`, and the DFG conversion of `a = ...`
succeeded, but `b = ...` failed, we still used to include `a = ...` in
the DFG, which then caused a spurious multi-driver error for `a` on
a subsequent DFG pass, as the original `{a, b} = ...` was still present
in the Ast, but we also had the extra `a = ...` from converting out of
DFG on the previous pass.

In this patch we only convert assignments with a concatenation on the
LHS, if all target LValues can be converted into DFG.

This is the proper fix for #4231
This commit is contained in:
Geza Lore 2025-08-03 14:52:20 +01:00 committed by GitHub
parent 36577bb549
commit deed20fb78
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 64 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

107
src/V3DfgAstToDfg.cpp
View File

@ -269,46 +269,67 @@ class AstToDfgVisitor final : public VNVisitor {
return {nullptr, 0};
}
// Build DfgEdge representing the LValue assignment. Returns false if unsuccessful.
// Convert assignment. Returns false if unsuccessful.
bool convertAssignment(FileLine* flp, AstNode* lhsp, DfgVertex* vtxp) {
// Simplify the LHS, to get rid of things like SEL(CONCAT(_, _), _)
lhsp = V3Const::constifyExpensiveEdit(lhsp);
// Represents a DFG assignment contributed by the AST assignment with the above 'lhsp'.
// There might be multiple of these if 'lhsp' is a concatenation.
struct Assignment final {
DfgVertexSplice* m_lhsp;
uint32_t m_idx;
DfgVertex* m_rhsp;
Assignment() = delete;
Assignment(DfgVertexSplice* lhsp, uint32_t idx, DfgVertex* rhsp)
: m_lhsp{lhsp}
, m_idx{idx}
, m_rhsp{rhsp} {}
};
// Concatenation on the LHS. Select parts of the driving 'vtxp' then convert each part
if (AstConcat* const concatp = VN_CAST(lhsp, Concat)) {
AstNode* const cLhsp = concatp->lhsp();
AstNode* const cRhsp = concatp->rhsp();
// Convert each concatenation LHS separately, gather all assignments
// we need to do into 'assignments', return true if all LValues
// converted successfully.
std::vector<Assignment> assignments;
const std::function<bool(AstNode*, DfgVertex*)> convertAllLValues
= [&](AstNode* lhsp, DfgVertex* vtxp) -> bool {
// Simplify the LHS, to get rid of things like SEL(CONCAT(_, _), _)
lhsp = V3Const::constifyExpensiveEdit(lhsp);
{ // Convet LHS of concat
// Concatenation on the LHS, convert each parts
if (AstConcat* const concatp = VN_CAST(lhsp, Concat)) {
AstNode* const cLhsp = concatp->lhsp();
AstNode* const cRhsp = concatp->rhsp();
// Convert Left of concat
FileLine* const lFlp = cLhsp->fileline();
DfgSel* const lVtxp = new DfgSel{*m_dfgp, lFlp, DfgVertex::dtypeFor(cLhsp)};
lVtxp->fromp(vtxp);
lVtxp->lsb(cRhsp->width());
if (!convertAssignment(flp, cLhsp, lVtxp)) return false;
}
{ // Convert RHS of concat
if (!convertAllLValues(cLhsp, lVtxp)) return false;
// Convert Rigth of concat
FileLine* const rFlp = cRhsp->fileline();
DfgSel* const rVtxp = new DfgSel{*m_dfgp, rFlp, DfgVertex::dtypeFor(cRhsp)};
rVtxp->fromp(vtxp);
rVtxp->lsb(0);
return convertAssignment(flp, cRhsp, rVtxp);
return convertAllLValues(cRhsp, rVtxp);
}
// Non-concatenation, convert the LValue
const auto pair = convertLValue(lhsp);
if (!pair.first) return false;
assignments.emplace_back(pair.first, pair.second, vtxp);
return true;
};
// Convert the given LHS assignment, give up if any LValues failed to convert
if (!convertAllLValues(lhsp, vtxp)) return false;
// All successful, connect the drivers
for (const Assignment& a : assignments) {
if (DfgSplicePacked* const spp = a.m_lhsp->template cast<DfgSplicePacked>()) {
spp->addDriver(flp, a.m_idx, a.m_rhsp);
} else if (DfgSpliceArray* const sap = a.m_lhsp->template cast<DfgSpliceArray>()) {
sap->addDriver(flp, a.m_idx, a.m_rhsp);
} else {
a.m_lhsp->v3fatalSrc("Unhandled DfgVertexSplice sub-type"); // LCOV_EXCL_LINE
}
}
// Construct LHS
const auto pair = convertLValue(lhsp);
if (!pair.first) return false;
// If successful connect the driver
if (DfgSplicePacked* const sPackedp = pair.first->template cast<DfgSplicePacked>()) {
sPackedp->addDriver(flp, pair.second, vtxp);
} else if (DfgSpliceArray* const sArrayp = pair.first->template cast<DfgSpliceArray>()) {
sArrayp->addDriver(flp, pair.second, vtxp);
} else {
lhsp->v3fatalSrc("Unhandled DfgVertexSplice sub-type"); // LCOV_EXCL_LINE
}
return true;
}
@ -455,9 +476,8 @@ class AstToDfgVisitor final : public VNVisitor {
const uint32_t bEnd = b.m_lsb + bWidth;
const uint32_t overlapEnd = std::min(aEnd, bEnd) - 1;
if (a.m_fileline->operatorCompare(*b.m_fileline) != 0
&& !varp->varp()->isUsedLoopIdx() // Loop index often abused, so suppress
) {
// Loop index often abused, so suppress
if (!varp->varp()->isUsedLoopIdx()) {
AstNode* const vp = varp->varScopep()
? static_cast<AstNode*>(varp->varScopep())
: static_cast<AstNode*>(varp->varp());
@ -608,22 +628,19 @@ class AstToDfgVisitor final : public VNVisitor {
const uint32_t bEnd = b.m_idx + bElements;
const uint32_t overlapEnd = std::min(aEnd, bEnd) - 1;
if (a.m_fileline->operatorCompare(*b.m_fileline) != 0) {
AstNode* const vp = varp->varScopep()
? static_cast<AstNode*>(varp->varScopep())
: static_cast<AstNode*>(varp->varp());
AstNode* const vp = varp->varScopep() ? static_cast<AstNode*>(varp->varScopep())
: static_cast<AstNode*>(varp->varp());
vp->v3warn( //
MULTIDRIVEN,
"Elements [" //
<< overlapEnd << ":" << b.m_idx << "] of signal '" << vp->prettyName()
<< sub << "' have multiple combinational drivers\n"
<< a.m_fileline->warnOther() << "... Location of first driver\n"
<< a.m_fileline->warnContextPrimary() << '\n'
<< b.m_fileline->warnOther() << "... Location of other driver\n"
<< b.m_fileline->warnContextSecondary() << vp->warnOther()
<< "... Only the first driver will be respected");
}
vp->v3warn( //
MULTIDRIVEN,
"Elements [" //
<< overlapEnd << ":" << b.m_idx << "] of signal '" << vp->prettyName()
<< sub << "' have multiple combinational drivers\n"
<< a.m_fileline->warnOther() << "... Location of first driver\n"
<< a.m_fileline->warnContextPrimary() << '\n'
<< b.m_fileline->warnOther() << "... Location of other driver\n"
<< b.m_fileline->warnContextSecondary() << vp->warnOther()
<< "... Only the first driver will be respected");
// If the first driver completely covers the range of the second driver,
// we can just delete the second driver completely, otherwise adjust the

2
test_regress/t/t_hier_block_chained.v
View File

@ -20,7 +20,9 @@ module t (clk);
reg [255:0] v1_5;
reg [255:0] v1_6;
reg [255:0] v1_7;
// verilator lint_off MULTIDRIVEN
reg [255:0] dummy;
// verilator lint_on MULTIDRIVEN
Calculate calc0(.clk(clk), .reset(reset), .v1_0(v1_0), .v1_1(dummy), .v1_2(dummy), .v1_3(dummy), .v1_4(dummy), .v1_5(dummy), .v1_6(dummy), .v1_7(dummy));
Calculate calc1(.clk(clk), .reset(reset), .v1_0(dummy), .v1_1(v1_1), .v1_2(dummy), .v1_3(dummy), .v1_4(dummy), .v1_5(dummy), .v1_6(dummy), .v1_7(dummy));