Optimize conditional merging across some impure statements (#7159)

- Allow reordering pure statements with DPI import calls iff no public
  variables (including those read via a DPI export) are involved. This
  ensures the DPI import can't observe the reordering
- Allow reordering of pure statements with AstDisplay and AstStop. This
  requires an assumption that AstDisplay and AstStop will not read or
  write model state other than via a VarRef explicitly present int the
  Ast.
Overall this allows eliminating a lot of conditionals around assertions,
which were previously not possible.
This commit is contained in:
Geza Lore 2026-03-01 10:47:05 +00:00 committed by GitHub
parent 26eac21432
commit 77ce9cec1e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 1394 additions and 1339 deletions

View File

@ -621,6 +621,8 @@ public:
init(text, setwidth);
}
ASTGEN_MEMBERS_AstCExpr;
void dump(std::ostream& str = std::cout) const override;
void dumpJson(std::ostream& str = std::cout) const override;
// METHODS
bool cleanOut() const override { return true; }
std::string emitC() override { V3ERROR_NA_RETURN(""); }

View File

@ -1869,6 +1869,14 @@ void AstCellInlineScope::dumpJson(std::ostream& str) const {
dumpJsonStrFunc(str, origModName);
dumpJsonGen(str);
}
void AstCExpr::dump(std::ostream& str) const {
this->AstNodeExpr::dump(str);
if (m_pure) str << " [PURE]";
}
void AstCExpr::dumpJson(std::ostream& str) const {
dumpJsonBoolIf(str, "pure", m_pure);
dumpJsonGen(str);
}
bool AstClass::isCacheableChild(const AstNode* nodep) {
return VN_IS(nodep, Var) || VN_IS(nodep, Typedef)
|| (VN_IS(nodep, Constraint) && !VN_AS(nodep, Constraint)->isExternProto())

View File

@ -137,12 +137,19 @@ struct StmtProperties final {
std::set<const AstVar*> m_rdVars; // Variables read by this statement
std::set<const AstVar*> m_wrVars; // Variables written by this statement
bool m_isFence = false; // Nothing should move across this statement, nor should it be merged
bool m_sideEffect = false; // Statement may have side effect, without access to model state
bool m_implPubRd = false; // Statement may implicitly read public state (without VarRef)
bool m_implPubWr = false; // Statement may implicitly write public state (without VarRef)
bool m_explPubRef = false; // Statement explicitly references public state via VarRef
AstNodeStmt* m_prevWithSameCondp = nullptr; // Previous node in same list, with same condition
bool writesConditionVar() const {
bool writesConditionVar(bool condPubWritable) const {
// This relies on MarkVarsVisitor having been called on the condition node
for (const AstVar* const varp : m_wrVars) {
if (varp->user1()) return true;
}
// If the condition contains a public variable, check if it might be written
if (condPubWritable && m_implPubWr) return true;
// Otherwise not written
return false;
}
};
@ -231,8 +238,12 @@ class CodeMotionAnalysisVisitor final : public VNVisitorConst {
// Add all rd/wr vars to outer statement
outerPropsp->m_rdVars.insert(m_propsp->m_rdVars.cbegin(), m_propsp->m_rdVars.cend());
outerPropsp->m_wrVars.insert(m_propsp->m_wrVars.cbegin(), m_propsp->m_wrVars.cend());
// If this statement is impure, the enclosing statement is also impure
if (m_propsp->m_isFence) outerPropsp->m_isFence = true;
// Propagate flags to enclosing statement
outerPropsp->m_isFence |= m_propsp->m_isFence;
outerPropsp->m_sideEffect |= m_propsp->m_sideEffect;
outerPropsp->m_implPubRd |= m_propsp->m_implPubRd;
outerPropsp->m_implPubWr |= m_propsp->m_implPubWr;
outerPropsp->m_explPubRef |= m_propsp->m_explPubRef;
}
}
@ -242,11 +253,44 @@ class CodeMotionAnalysisVisitor final : public VNVisitorConst {
// Gather read and written variables
if (access.isReadOrRW()) m_propsp->m_rdVars.insert(varp);
if (access.isWriteOrRW()) m_propsp->m_wrVars.insert(varp);
if (varp->isSigPublic() || varp->isWrittenByDpi() || varp->isReadByDpi()) {
m_propsp->m_explPubRef = true;
}
}
void checkProperties(AstNode* nodep) {
// Ignore StmtExpr. It interferes with special casing DPI calls below,
// and it only checks if the child expr is impure, which is checked
// explicitly during the analysis.
if (VN_IS(nodep, StmtExpr)) return;
// Call to a DPI import
if (const AstNodeCCall* const ccallp = VN_CAST(nodep, NodeCCall)) {
AstCFunc* const funcp = ccallp->funcp();
if (funcp->dpiImportWrapper()) {
if (!funcp->dpiPure()) {
// Assume has side effect and accesses public state
m_propsp->m_sideEffect = true;
m_propsp->m_implPubRd = true;
m_propsp->m_implPubWr = true;
}
return;
}
}
// Side effect, but assume does not access public state
if (VN_IS(nodep, Display) || VN_IS(nodep, Stop)) {
m_propsp->m_sideEffect = true;
return;
}
// If impure, or branch, mark statement as fence
if (!nodep->isPure() || nodep->isBrancher()) m_propsp->m_isFence = true;
}
void analyzeNode(AstNode* nodep) {
// If impure, or branch, mark statement as fence
if (m_propsp && (!nodep->isPure() || nodep->isBrancher())) m_propsp->m_isFence = true;
// Check properties of this node if under a statement
if (m_propsp) checkProperties(nodep);
// Analyze children
iterateChildrenConst(nodep);
}
@ -310,6 +354,13 @@ class CodeMotionOptimizeVisitor final : public VNVisitor {
// Don't move across fences
if (aProps.m_isFence) return false;
if (bProps.m_isFence) return false;
// Don't swap side effecting statements, but can move others across them
if (aProps.m_sideEffect && bProps.m_sideEffect) return false;
// Don't swap if there is a hazard around public state - must observe ordering
const bool bPubRef = bProps.m_implPubWr || bProps.m_implPubRd || bProps.m_explPubRef;
if (aProps.m_implPubWr && bPubRef) return false;
const bool aPubRef = aProps.m_implPubWr || aProps.m_implPubRd || aProps.m_explPubRef;
if (aPubRef && bProps.m_implPubWr) return false;
// If either statement writes a variable that the other reads, they are not swappable
if (!areDisjoint(aProps.m_rdVars, bProps.m_wrVars)) return false;
if (!areDisjoint(bProps.m_rdVars, aProps.m_wrVars)) return false;
@ -446,6 +497,7 @@ class MergeCondVisitor final : public VNVisitor {
AstNodeExpr* m_mgCondp = nullptr; // The condition of the first node
const AstNode* m_mgLastp = nullptr; // Last node in merged sequence
const AstNode* m_mgNextp = nullptr; // Next node in list being examined
bool m_mgCondPubWritable = false; // True if m_mgCondp contains a public writable variable
uint32_t m_listLenght = 0; // Length of current list
std::queue<AstNode*>* m_workQueuep = nullptr; // Node lists (via AstNode::nextp()) to merge
@ -717,6 +769,7 @@ class MergeCondVisitor final : public VNVisitor {
m_mgCondp = nullptr;
m_mgLastp = nullptr;
m_mgNextp = nullptr;
m_mgCondPubWritable = false;
AstNode::user1ClearTree(); // Clear marked variables
AstNode::user2ClearTree();
// Merge recursively within the branches of an un-merged AstNodeIF
@ -753,14 +806,19 @@ class MergeCondVisitor final : public VNVisitor {
// Set up head of new list if node is first in list
if (!m_mgFirstp) {
UASSERT_OBJ(condp, nodep, "Cannot start new list without condition");
// Mark variable references in the condition
condp->foreach([](const AstVarRef* nodep) { nodep->varp()->user1(1); });
// Mark variable references in the condition, record if a public variable is involved
condp->foreach([&](const AstVarRef* nodep) {
AstVar* const varp = nodep->varp();
varp->user1(1);
if (varp->isSigPublic() || varp->isWrittenByDpi()) m_mgCondPubWritable = true;
});
// Now check again if mergeable. We need this to pick up assignments to conditions,
// e.g.: 'c = c ? a : b' at the beginning of the list, which is in fact not mergeable
// because it updates the condition. We simply bail on these.
if ((*m_stmtPropertiesp)(nodep).writesConditionVar()) {
if ((*m_stmtPropertiesp)(nodep).writesConditionVar(m_mgCondPubWritable)) {
// Clear marked variables
AstNode::user1ClearTree();
m_mgCondPubWritable = false;
// We did not add to the list
return false;
}
@ -773,7 +831,7 @@ class MergeCondVisitor final : public VNVisitor {
AstNodeStmt* const backp = VN_CAST(m_mgFirstp->backp(), NodeStmt);
if (!backp || backp->nextp() != m_mgFirstp) break; // Don't move up the tree
const StmtProperties& props = (*m_stmtPropertiesp)(backp);
if (props.m_isFence || props.writesConditionVar()) break;
if (props.m_isFence || props.writesConditionVar(m_mgCondPubWritable)) break;
if (isSimplifiableNode(backp)) {
++m_listLenght;
m_mgFirstp = backp;
@ -824,7 +882,7 @@ class MergeCondVisitor final : public VNVisitor {
if (props.m_isFence) return false; // Fence node never mergeable
// If the statement writes a condition variable of a pending merge,
// we must end the pending merge
if (m_mgFirstp && props.writesConditionVar()) mergeEnd();
if (m_mgFirstp && props.writesConditionVar(m_mgCondPubWritable)) mergeEnd();
return true; // Now surely mergeable
}

View File

@ -765,7 +765,8 @@ class TaskVisitor final : public VNVisitor {
// __Vscopep
ccallp->addArgsp(snp);
// __Vfilenamep
ccallp->addArgsp(new AstCExpr{flp, "\"" + flp->filenameEsc() + "\"", 64});
ccallp->addArgsp(
new AstCExpr{flp, AstCExpr::Pure{}, "\"" + flp->filenameEsc() + "\"", 64});
// __Vlineno
ccallp->addArgsp(new AstConst(flp, flp->lineno()));
}

File diff suppressed because it is too large Load Diff

View File

@ -17,8 +17,8 @@ test.execute()
if test.vlt:
# Note, with vltmt this might be split differently, so only checking vlt
test.file_grep(test.stats, r'Optimizations, MergeCond merges\s+(\d+)', 9)
test.file_grep(test.stats, r'Optimizations, MergeCond merged items\s+(\d+)', 580)
test.file_grep(test.stats, r'Optimizations, MergeCond longest merge\s+(\d+)', 128)
test.file_grep(test.stats, r'Optimizations, MergeCond merges\s+(\d+)', 11)
test.file_grep(test.stats, r'Optimizations, MergeCond merged items\s+(\d+)', 585)
test.file_grep(test.stats, r'Optimizations, MergeCond longest merge\s+(\d+)', 129)
test.passes()

View File

@ -0,0 +1,29 @@
// -*- mode: C++; c-file-style: "cc-mode" -*-
//*************************************************************************
//
// This program is free software; you can redistribute it and/or modify it
// under the terms of either the GNU Lesser General Public License Version 3
// or the Perl Artistic License Version 2.0.
// SPDX-FileCopyrightText: 2026-2026 Wilson Snyder
// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
//
//*************************************************************************
#include "svdpi.h"
#include <iostream>
extern "C" void setDpi(int value);
extern "C" void setViaDpi(int value) {
std::cout << "setViaDpi " << value << std::endl;
setDpi(value);
}
extern "C" int getDpi();
extern "C" int getViaDpi() {
const int value = getDpi();
std::cout << "getViaDpi " << value << std::endl;
return value;
}

View File

@ -0,0 +1,173 @@
setViaDpi 3
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 4 pub= 1
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt= 8 pub= 2
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 18
setViaDpi 34
setViaDpi 32
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 12 pub= 3
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 35
setViaDpi 67
setViaDpi 3
getViaDpi 2
cnt= 16 pub= 4
setViaDpi 6
getViaDpi 2
cnt= 20 pub= 5
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt= 24 pub= 6
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 22
setViaDpi 38
setViaDpi 36
setViaDpi 3
setViaDpi 6
getViaDpi 2
cnt= 28 pub= 7
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 39
setViaDpi 71
getViaDpi 2
cnt= 32 pub= 8
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 36 pub= 9
getViaDpi 24
getViaDpi 24
setViaDpi 3
getViaDpi 100
cnt= 40 pub= 10
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 26
setViaDpi 42
setViaDpi 40
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 44 pub= 11
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 43
setViaDpi 75
getViaDpi 2
cnt= 48 pub= 12
setViaDpi 3
setViaDpi 6
getViaDpi 2
cnt= 52 pub= 13
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt= 56 pub= 14
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 30
setViaDpi 46
setViaDpi 44
setViaDpi 6
getViaDpi 2
cnt= 60 pub= 15
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 47
setViaDpi 79
setViaDpi 3
getViaDpi 2
cnt= 64 pub= 16
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 68 pub= 17
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt= 72 pub= 18
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 34
setViaDpi 50
setViaDpi 48
setViaDpi 3
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt= 76 pub= 19
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 51
setViaDpi 83
getViaDpi 2
cnt= 80 pub= 20
setViaDpi 6
getViaDpi 2
cnt= 84 pub= 21
getViaDpi 24
getViaDpi 24
setViaDpi 3
getViaDpi 100
cnt= 88 pub= 22
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 38
setViaDpi 54
setViaDpi 52
setViaDpi 6
getViaDpi 2
cnt= 92 pub= 23
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 55
setViaDpi 87
getViaDpi 2
cnt= 96 pub= 24
setViaDpi 3
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt=100 pub= 25
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt=104 pub= 26
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 42
setViaDpi 58
setViaDpi 56
setViaDpi 6
setViaDpi 4
getViaDpi 2
cnt=108 pub= 27
cyc[1] is 1 once
cyc[1] is 1 twice
setViaDpi 59
setViaDpi 91
setViaDpi 3
getViaDpi 2
cnt=112 pub= 28
setViaDpi 6
getViaDpi 2
cnt=116 pub= 29
getViaDpi 24
getViaDpi 24
getViaDpi 100
cnt=120 pub= 30
*-* All Finished *-*

View File

@ -0,0 +1,20 @@
#!/usr/bin/env python3
# DESCRIPTION: Verilator: Verilog Test driver/expect definition
#
# This program is free software; you can redistribute it and/or modify it
# under the terms of either the GNU Lesser General Public License Version 3
# or the Perl Artistic License Version 2.0.
# SPDX-FileCopyrightText: 2026 Wilson Snyder
# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
import vltest_bootstrap
test.scenarios('vlt_all')
test.compile(verilator_flags2=["--binary", "--stats", test.top_filename.replace(".v", ".cpp")])
test.file_grep(test.stats, r'Optimizations, MergeCond merges\s+(\d+)', 4)
test.execute(expect_filename=test.golden_filename)
test.passes()

View File

@ -0,0 +1,133 @@
// DESCRIPTION: Verilator: Verilog Test module
//
// This file ONLY is placed under the Creative Commons Public Domain.
// SPDX-FileCopyrightText: 2026 Wilson Snyder
// SPDX-License-Identifier: CC0-1.0
`define stop $stop
`define check(got ,exp) do if ((got) !== (exp)) begin $write("%%Error: %s:%0d: cyc=%0d got='h%x exp='h%x\n", `__FILE__,`__LINE__, cyc, (got), (exp)); `stop; end while(0)
module t;
logic clk = 0;
always #5 clk = ~clk;
initial begin
#300;
$write("*-* All Finished *-*\n");
$finish;
end
int cyc = 0;
always @(posedge clk) cyc <= cyc + 1;
int dpiWr = 0;
function automatic void setDpi(int value);
dpiWr = value;
endfunction
export "DPI-C" function setDpi;
import "DPI-C" context function void setViaDpi(int value); // calls setDpi(value)
int dpiRd = 0;
function automatic int getDpi();
return dpiRd;
endfunction
export "DPI-C" function getDpi;
import "DPI-C" context function int getViaDpi(); // calls getDpi()
int tmp;
int cnt = 0;
int pub /* verilator public_flat_rd */ = 0;
always @(posedge clk) begin
//---------------------------
// Mergeable
// Side effect but no implicit state access.
if (cyc[1]) $display("cyc[1] is 1 once");
++cnt;
if (cyc[1]) $display("cyc[1] is 1 twice");
// Side effect but no implicit state access.
if (cyc > 100000) $error("cyc > 100000 once");
++cnt;
if (cyc > 100000) $error("cyc > 100000 once");
// DPI call, but no public state involved.
dpiWr = 13;
if (cyc[1:0] == 2'd2) setViaDpi(cyc + 16);
++cnt;
if (cyc[1:0] == 2'd2) setViaDpi(cyc + 32);
`check(dpiWr, cyc % 4 == 2 ? cyc + 32 : 13);
// DPI call, but no public state involved.
dpiRd = 24;
tmp = 10;
if (cyc[1:0] == 2'd1) begin
tmp = getViaDpi();
tmp += 10;
end
++cnt;
if (cyc[1:0] == 2'd1) begin
tmp = getViaDpi();
tmp += 20;
end
`check(tmp, cyc % 4 == 1 ? 44 : 10);
//---------------------------
// NOT Mergeable
// DPI call, possible implicit state chagne.
tmp = dpiWr;
if (dpiWr[1:0] == 2'd2) setViaDpi(dpiWr & ~32'b11);
if (dpiWr[1:0] == 2'd2) setViaDpi(dpiWr + 10); // Won't execute
`check(dpiWr, cyc % 4 == 2 ? (tmp & ~32'b11) : 13);
// DPI call, possible implicit state acces.
dpiWr = 14;
if (cyc[1:0] == 2'd3) setViaDpi(cyc + 32);
++pub;
if (cyc[1:0] == 2'd3) setViaDpi(cyc + 64);
`check(dpiWr, cyc % 4 == 3 ? cyc + 64 : 14);
// DPI call, possible implicit state change.
dpiWr = 11;
tmp = cyc + $c(0); // Prevent repalcing with 'cyc'
if (tmp % 3 == 0) begin
setViaDpi(3);
tmp = dpiWr + 2;
end
if (tmp % 3 == 0) setViaDpi(4); // Won't execute
`check(dpiWr, cyc % 3 == 0 ? 3 : 11);
dpiWr = 3;
// DPI call, possible implicit state change.
tmp = cyc + $c(0); // Prevent repalcing with 'cyc'
if (tmp % 2 == 0) begin
setViaDpi(6);
if (cyc[2]) tmp = dpiWr + 1;
end
if (tmp % 2 == 0) setViaDpi(4); // Sometime executes
`check(tmp, cyc % 2 == 0 ? (cyc[2] ? 7 : cyc) : cyc);
`check(dpiWr, cyc % 2 == 0 ? (cyc[2] ? 6 : 4) : 3);
// DPI call, possible implicit state read.
dpiRd = 2;
if (cyc[1:0] == 2'd1) begin
dpiRd = 100;
end
tmp = getViaDpi();
if (cyc[1:0] == 2'd1) begin
dpiRd = 3;
end
`check(tmp, cyc % 4 == 1 ? 100 : 2);
`check(dpiRd, cyc % 4 == 1 ? 3 : 2);
//---------------------------
// Dispaly so not eliminated
$display("cnt=%3d pub=%3d", cnt, pub);
end
endmodule