From bdae48f6ae16235266edaefe016ddf6b2aeeee60 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Mon, 29 Sep 2025 08:18:54 -0400 Subject: [PATCH] Optimize duplicate 'if' and '?:' conditions (#3807) (#6495) --- src/V3Const.cpp | 100 ++++++++++++++++++ test_regress/t/t_case_duplicated_if.v | 57 ---------- ...ed_if.py => t_opt_const_cond_redundant.py} | 5 +- test_regress/t/t_opt_const_cond_redundant.v | 55 ++++++++++ 4 files changed, 159 insertions(+), 58 deletions(-) delete mode 100644 test_regress/t/t_case_duplicated_if.v rename test_regress/t/{t_case_duplicated_if.py => t_opt_const_cond_redundant.py} (68%) create mode 100644 test_regress/t/t_opt_const_cond_redundant.v diff --git a/src/V3Const.cpp b/src/V3Const.cpp index fa199d4aa..15b834f59 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -933,6 +933,8 @@ class ConstVisitor final : public VNVisitor { const AstNode* m_scopep = nullptr; // Current scope const AstAttrOf* m_attrp = nullptr; // Current attribute VDouble0 m_statBitOpReduction; // Ops reduced in ConstBitOpTreeVisitor + VDouble0 m_statCondExprRedundant; // Conditional repeated expressions + VDouble0 m_statIfCondExprRedundant; // Conditional repeated expressions const bool m_globalPass; // ConstVisitor invoked as a global pass static uint32_t s_globalPassNum; // Counts number of times ConstVisitor invoked as global pass V3UniqueNames m_concswapNames; // For generating unique temporary variable names @@ -1117,6 +1119,99 @@ class ConstVisitor final : public VNVisitor { VL_DO_DANGLING(pushDeletep(nodep), nodep); return true; } + + bool matchCondCond(AstCond* nodep) { + // Same condition on either leg of a condition means that + // expression is either always true or always false + if (VN_IS(nodep->backp(), Cond)) return false; // Was checked when visited parent + if (!VN_IS(nodep->thenp(), Cond) && !VN_IS(nodep->elsep(), Cond)) + return false; // Short circuit + std::vector truesp; + std::vector falsesp; + matchCondCondRecurse(nodep, truesp /*ref*/, falsesp /*ref*/); + return false; // Can optimize further + } + void matchCondCondRecurse(AstCond* nodep, std::vector& truesp, + std::vector& falsesp) { + // Avoid O(n^2) compares + // Could reduce cost with hash table, but seems unlikely to be worth cost + if (truesp.size() > 4 || falsesp.size() > 4) return; + if (!nodep->condp()->isPure()) return; + bool replaced = false; + for (AstNodeExpr* condp : truesp) { + if (replaced) break; + if (!operandsSame(nodep->condp(), condp)) continue; + UINFO(9, "COND(c, CONDb(c, tt, tf), f) -> CONDb(1, tt, tf) " << nodep); + replaceNum(nodep->condp(), 1); + replaced = true; + ++m_statCondExprRedundant; + } + for (AstNodeExpr* condp : falsesp) { + if (replaced) break; + if (!operandsSame(nodep->condp(), condp)) continue; + UINFO(9, "COND(c, t, CONDb(c, ft, ff)) -> CONDb(0, ft, ff) " << nodep); + replaceZero(nodep->condp()); + replaced = true; + ++m_statCondExprRedundant; + } + if (AstCond* subCondp = VN_CAST(nodep->thenp(), Cond)) { + if (!replaced) truesp.emplace_back(nodep->condp()); + matchCondCondRecurse(subCondp, truesp /*ref*/, falsesp /*ref*/); + if (!replaced) truesp.pop_back(); + } + if (AstCond* subCondp = VN_CAST(nodep->elsep(), Cond)) { + if (!replaced) falsesp.emplace_back(nodep->condp()); + matchCondCondRecurse(subCondp, truesp /*ref*/, falsesp /*ref*/); + if (!replaced) falsesp.pop_back(); + } + } + void matchIfCondCond(AstNodeIf* nodep) { + // Same condition on either leg of a condition means that + // expression is either always true or always false + if (VN_IS(nodep->backp(), If)) return; // Was checked when visited parent + if (!VN_IS(nodep->thensp(), If) && !VN_IS(nodep->elsesp(), If)) return; // Short circuit + std::vector truesp; + std::vector falsesp; + matchIfCondCondRecurse(nodep, truesp /*ref*/, falsesp /*ref*/); + } + void matchIfCondCondRecurse(AstNodeIf* nodep, std::vector& truesp, + std::vector& falsesp) { + // Avoid O(n^2) compares + // Could reduce cost with hash table, but seems unlikely to be worth cost + if (truesp.size() > 4 || falsesp.size() > 4) return; + if (!nodep->condp()->isPure()) return; + bool replaced = false; + for (AstNodeExpr* condp : truesp) { + if (replaced) break; + if (!operandsSame(nodep->condp(), condp)) continue; + UINFO(9, "COND(c, CONDb(c, tt, tf), f) -> CONDb(1, tt, tf) " << nodep); + replaceNum(nodep->condp(), 1); + replaced = true; + ++m_statIfCondExprRedundant; + } + for (AstNodeExpr* condp : falsesp) { + if (replaced) break; + if (!operandsSame(nodep->condp(), condp)) continue; + UINFO(9, "COND(c, t, CONDb(c, ft, ff)) -> CONDb(0, ft, ff) " << nodep); + replaceZero(nodep->condp()); + replaced = true; + ++m_statIfCondExprRedundant; + } + // We only check the first statement of parent IF is an If + // So we don't need to check for effects in the executing thensp/elsesp + // altering the child't condition. e.g. 'if (x) begin x=1; if (x) end' + if (AstNodeIf* subIfp = VN_CAST(nodep->thensp(), NodeIf)) { + if (!replaced) truesp.emplace_back(nodep->condp()); + matchIfCondCondRecurse(subIfp, truesp /*ref*/, falsesp /*ref*/); + if (!replaced) truesp.pop_back(); + } + if (AstNodeIf* subIfp = VN_CAST(nodep->elsesp(), NodeIf)) { + if (!replaced) falsesp.emplace_back(nodep->condp()); + matchIfCondCondRecurse(subIfp, truesp /*ref*/, falsesp /*ref*/); + if (!replaced) falsesp.pop_back(); + } + } + bool matchMaskedOr(AstAnd* nodep) { // Masking an OR with terms that have no bits set under the mask is replaced with masking // only the remaining terms. Canonical example as generated by V3Expand is: @@ -3340,6 +3435,7 @@ class ConstVisitor final : public VNVisitor { } else { // Optimizations that don't reform the IF itself if (operandBoolShift(nodep->condp())) replaceBoolShift(nodep->condp()); + matchIfCondCond(nodep); } } } @@ -3702,6 +3798,7 @@ class ConstVisitor final : public VNVisitor { TREEOPA("AstCond{$condp.isZero, $thenp.castConst, $elsep.castConst}", "replaceWChild(nodep,$elsep)"); TREEOPA("AstCond{$condp.isNeqZero, $thenp.castConst, $elsep.castConst}", "replaceWChild(nodep,$thenp)"); TREEOP ("AstCond{$condp, operandsSame($thenp,,$elsep)}","replaceWChild(nodep,$thenp)"); + TREEOP ("AstCond{$condp, matchCondCond(nodep)}", "DONE") // Same condition then skip // This visit function here must allow for short-circuiting. TREEOPS("AstCond{$condp.isZero}", "replaceWIteratedThs(nodep)"); TREEOPS("AstCond{$condp.isNeqZero}", "replaceWIteratedRhs(nodep)"); @@ -3996,6 +4093,9 @@ public: V3Stats::addStatSum("Optimizations, Const bit op reduction", m_statBitOpReduction); } } + V3Stats::addStatSum("Optimizations, Cond redundant expressions", m_statCondExprRedundant); + V3Stats::addStatSum("Optimizations, If cond redundant expressions", + m_statIfCondExprRedundant); } AstNode* mainAcceptEdit(AstNode* nodep) { diff --git a/test_regress/t/t_case_duplicated_if.v b/test_regress/t/t_case_duplicated_if.v deleted file mode 100644 index 822b2f884..000000000 --- a/test_regress/t/t_case_duplicated_if.v +++ /dev/null @@ -1,57 +0,0 @@ -// DESCRIPTION: Verilator: Verilog Test module -// -// This file ONLY is placed under the Creative Commons Public Domain, for -// any use, without warranty, 2023 by Wilson Snyder. -// SPDX-License-Identifier: CC0-1.0 - -// bug3806 - -module t (/*AUTOARG*/ - // Inputs - clk - ); - - input clk; - - reg [65:0] idx /*verilator public*/; initial idx = 1; - - wire unlikely = idx > 200; - - typedef enum logic {UP, DOWN} dir_t; - - dir_t direction; - - always_comb direction = idx % 2 == 0 ? UP : DOWN; - - int ups; // Make computable - - always @(posedge clk) begin - if (idx > 100) begin -`ifdef TEST_VERBOSE - $write("ups = %0d\n", ups); -`endif - if (ups != 50049) $stop; - $write("*-* All Finished *-*\n"); - $finish; - end - - if (direction == UP) - ++ups; - else if (direction == UP) - ++ups; - else - ups += 1000; - - case (direction) - DOWN: idx = idx+3; - UP: idx = idx-1; - default: begin - // This if just gets rid of branch pred on default^ - if (unlikely == '1) begin - $write("never\n"); - end - end - endcase - end - -endmodule diff --git a/test_regress/t/t_case_duplicated_if.py b/test_regress/t/t_opt_const_cond_redundant.py similarity index 68% rename from test_regress/t/t_case_duplicated_if.py rename to test_regress/t/t_opt_const_cond_redundant.py index d4f986441..5a1640c4e 100755 --- a/test_regress/t/t_case_duplicated_if.py +++ b/test_regress/t/t_opt_const_cond_redundant.py @@ -11,8 +11,11 @@ import vltest_bootstrap test.scenarios('simulator') -test.compile() +test.compile(verilator_flags2=['--stats']) test.execute() +test.file_grep(test.stats, r'Optimizations, Cond redundant expressions\s+(\d+)', 1) +test.file_grep(test.stats, r'Optimizations, If cond redundant expressions\s+(\d+)', 1) + test.passes() diff --git a/test_regress/t/t_opt_const_cond_redundant.v b/test_regress/t/t_opt_const_cond_redundant.v new file mode 100644 index 000000000..cf1c89ba2 --- /dev/null +++ b/test_regress/t/t_opt_const_cond_redundant.v @@ -0,0 +1,55 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +// bug3806 + +module t ( + input clk +); + + reg [65:0] idx /*verilator public*/; + initial idx = 1; + + wire unlikely = idx > 200; + + typedef enum logic { + UP, + DOWN + } dir_t; + + dir_t direction; + + always_comb direction = idx % 2 == 0 ? UP : DOWN; + + int ups; // Make computable + + always @(posedge clk) begin + if (idx > 100) begin +`ifdef TEST_VERBOSE + $write("ups = %0d\n", ups); +`endif + if (ups != 50049) $stop; + $write("*-* All Finished *-*\n"); + $finish; + end + + if (direction == UP) ++ups; + else if (direction == UP) ++ups; + else ups += 1000; + + case (direction) + DOWN: idx = idx + 3; + UP: idx = idx - 1; + default: begin + // This if just gets rid of branch pred on default^ + if (unlikely == '1) begin + $write("never\n"); + end + end + endcase + end + +endmodule