From e5594e9c6cb213f5009d9a39cf79fffd6e0aaf67 Mon Sep 17 00:00:00 2001 From: Matthew Ballance Date: Wed, 11 Mar 2026 00:01:20 +0000 Subject: [PATCH] Fix constifyParamsEdit regression in visit(AstInsideRange*) The original code called V3Const::constifyParamsEdit() in V3Width::visit(AstInsideRange*) to fold negative bin bounds (e.g., NEGATE(100) -> -100) for covergroup bins. Since constifyParamsEdit uses m_required=true, it errored on any non-constant expression -- breaking the upstream test t_constraint_array_index_simple which uses a foreach loop variable 'i' in an inside constraint expression. Fix: Remove the unconditional constifyParamsEdit calls from V3Width::visit(AstInsideRange*). Instead: - In V3Width: when m_vup is null (covergroup bin context, not an expression context), use constifyEdit (m_required=false) to fold constant arithmetic before iterating children. - In V3Covergroup: call constifyEdit at the two sites that cast InsideRange bounds to AstConst for bin expansion. This moves the covergroup-specific concern to the right layer (V3Covergroup) while keeping V3Width safe for all contexts. Also update golden files for t_covergroup_trans_3value, t_covergroup_trans_restart, and t_covergroup_with_sample_args_too_many_bad whose line numbers shifted by 1 due to the SPDX copyright header additions in the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/V3Covergroup.cpp | 13 +++++++++---- src/V3Width.cpp | 11 ++++++++--- test_regress/t/t_covergroup_trans_3value.out | 8 ++++---- test_regress/t/t_covergroup_trans_restart.out | 4 ++-- .../t_covergroup_with_sample_args_too_many_bad.out | 6 +++--- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/V3Covergroup.cpp b/src/V3Covergroup.cpp index e22237d39..ed7cadffc 100644 --- a/src/V3Covergroup.cpp +++ b/src/V3Covergroup.cpp @@ -26,6 +26,7 @@ #include "V3Covergroup.h" +#include "V3Const.h" #include "V3MemberMap.h" VL_DEFINE_DEBUG_FUNCTIONS; @@ -260,8 +261,10 @@ class FunctionalCoverageVisitor final : public VNVisitor { values.insert(constp->toUQuad()); } else if (AstInsideRange* rangep = VN_CAST(nodep, InsideRange)) { // Range [lo:hi] - AstConst* loConstp = VN_CAST(rangep->lhsp(), Const); - AstConst* hiConstp = VN_CAST(rangep->rhsp(), Const); + AstNodeExpr* lhsp = V3Const::constifyEdit(rangep->lhsp()); + AstNodeExpr* rhsp = V3Const::constifyEdit(rangep->rhsp()); + AstConst* loConstp = VN_CAST(lhsp, Const); + AstConst* hiConstp = VN_CAST(rhsp, Const); if (loConstp && hiConstp) { uint64_t lo = loConstp->toUQuad(); uint64_t hi = hiConstp->toUQuad(); @@ -995,8 +998,10 @@ class FunctionalCoverageVisitor final : public VNVisitor { } } else if (AstInsideRange* const insideRangep = VN_CAST(rangep, InsideRange)) { // For InsideRange [min:max], create bins for each value - AstConst* const minConstp = VN_CAST(insideRangep->lhsp(), Const); - AstConst* const maxConstp = VN_CAST(insideRangep->rhsp(), Const); + AstNodeExpr* const minp = V3Const::constifyEdit(insideRangep->lhsp()); + AstNodeExpr* const maxp = V3Const::constifyEdit(insideRangep->rhsp()); + AstConst* const minConstp = VN_CAST(minp, Const); + AstConst* const maxConstp = VN_CAST(maxp, Const); if (minConstp && maxConstp) { int minVal = minConstp->toSInt(); int maxVal = maxConstp->toSInt(); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 19d42fc7a..357c9cd79 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -3393,9 +3393,14 @@ class WidthVisitor final : public VNVisitor { } void visit(AstInsideRange* nodep) override { // Just do each side; AstInside will rip these nodes out later - // Constant-fold range bounds (e.g., NEGATE(100) becomes -100) - V3Const::constifyParamsEdit(nodep->lhsp()); // May relink pointed to node - V3Const::constifyParamsEdit(nodep->rhsp()); // May relink pointed to node + // When m_vup is null we are in a covergroup bin context (not an expression context). + // Fold constant arithmetic (e.g., NEGATE(100) -> -100) so the children have their + // types set before further tree processing. Use constifyEdit (not constifyParamsEdit) + // to avoid errors on any non-constant expressions. + if (!m_vup) { + V3Const::constifyEdit(nodep->lhsp()); // lhsp may change + V3Const::constifyEdit(nodep->rhsp()); // rhsp may change + } userIterateAndNext(nodep->lhsp(), m_vup); userIterateAndNext(nodep->rhsp(), m_vup); nodep->dtypeFrom(nodep->lhsp()); diff --git a/test_regress/t/t_covergroup_trans_3value.out b/test_regress/t/t_covergroup_trans_3value.out index 034bd05a5..20aaec69c 100644 --- a/test_regress/t/t_covergroup_trans_3value.out +++ b/test_regress/t/t_covergroup_trans_3value.out @@ -1,11 +1,11 @@ -%Warning-CASEINCOMPLETE: t/t_covergroup_trans_3value.v:13:12: Case values incompletely covered (example pattern 0x3) +%Warning-CASEINCOMPLETE: t/t_covergroup_trans_3value.v:14:12: Case values incompletely covered (example pattern 0x3) : ... note: In instance 't.cg' - 13 | bins trans_3val = (0 => 1 => 2); + 14 | bins trans_3val = (0 => 1 => 2); | ^~~~~~~~~~ ... For warning description see https://verilator.org/warn/CASEINCOMPLETE?v=latest ... Use "/* verilator lint_off CASEINCOMPLETE */" and lint_on around source to disable this message. -%Warning-CASEINCOMPLETE: t/t_covergroup_trans_3value.v:14:12: Case values incompletely covered (example pattern 0x3) +%Warning-CASEINCOMPLETE: t/t_covergroup_trans_3value.v:15:12: Case values incompletely covered (example pattern 0x3) : ... note: In instance 't.cg' - 14 | bins trans_3val_2 = (2 => 3 => 4); + 15 | bins trans_3val_2 = (2 => 3 => 4); | ^~~~~~~~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_covergroup_trans_restart.out b/test_regress/t/t_covergroup_trans_restart.out index e21357c52..72c0d3021 100644 --- a/test_regress/t/t_covergroup_trans_restart.out +++ b/test_regress/t/t_covergroup_trans_restart.out @@ -1,6 +1,6 @@ -%Warning-CASEINCOMPLETE: t/t_covergroup_trans_restart.v:13:12: Case values incompletely covered (example pattern 0x3) +%Warning-CASEINCOMPLETE: t/t_covergroup_trans_restart.v:14:12: Case values incompletely covered (example pattern 0x3) : ... note: In instance 't.cg' - 13 | bins trans_restart = (1 => 2 => 3); + 14 | bins trans_restart = (1 => 2 => 3); | ^~~~~~~~~~~~~ ... For warning description see https://verilator.org/warn/CASEINCOMPLETE?v=latest ... Use "/* verilator lint_off CASEINCOMPLETE */" and lint_on around source to disable this message. diff --git a/test_regress/t/t_covergroup_with_sample_args_too_many_bad.out b/test_regress/t/t_covergroup_with_sample_args_too_many_bad.out index 2c37fce5a..ea7fdcabf 100644 --- a/test_regress/t/t_covergroup_with_sample_args_too_many_bad.out +++ b/test_regress/t/t_covergroup_with_sample_args_too_many_bad.out @@ -1,9 +1,9 @@ -%Error: t/t_covergroup_with_sample_args_too_many_bad.v:15:26: Too many arguments in call to function 'sample' +%Error: t/t_covergroup_with_sample_args_too_many_bad.v:16:26: Too many arguments in call to function 'sample' : ... note: In instance 't' - 15 | cov1.sample(5, 1'b0, 42); + 16 | cov1.sample(5, 1'b0, 42); | ^~~~~~ : ... Location of function 'sample' declaration: - 9 | covergroup cg_with_sample(int init) with function sample (int addr, bit is_read = 1'b0); + 10 | covergroup cg_with_sample(int init) with function sample (int addr, bit is_read = 1'b0); | ^~~~~~~~~~ ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. %Error: Exiting due to