From b0141613590165be1c44f719df4145de6a787dcd Mon Sep 17 00:00:00 2001 From: Matthew Ballance Date: Tue, 9 Jun 2026 00:40:21 +0000 Subject: [PATCH] Incorporate feedback comments Signed-off-by: Matthew Ballance --- include/verilated_cov_model.h | 28 ++++++------ include/verilated_covergroup.cpp | 35 ++++++++------- include/verilated_covergroup.h | 75 +++++++++++++++++++++++--------- src/V3AstAttr.h | 13 ++++++ src/V3Covergroup.cpp | 30 ++++--------- 5 files changed, 108 insertions(+), 73 deletions(-) diff --git a/include/verilated_cov_model.h b/include/verilated_cov_model.h index ac34809d5..efcbf20bb 100644 --- a/include/verilated_cov_model.h +++ b/include/verilated_cov_model.h @@ -14,14 +14,9 @@ /// \file /// \brief Verilated functional-coverage model interfaces /// -/// Read-side reflection seam: abstract interfaces a coverage writer queries, -/// implemented by the collection runtime (VlCoverpoint, and later VlCoverCross -/// and the covergroup type/instance nodes). Carries no implementation and -/// depends on nothing in verilated_cov.h, so a writer or a second back-end can -/// compile against it independently. -/// -/// Step 1 declares only the slice the runtime implements today; it grows a -/// slice per phase as consumers (writer, cross) arrive. +/// Defines interface classes to runtime covergroup coverage-collection classes. +/// These are used to query coverage achievement at runtime, and (future) +/// when writing coverage to the coverage database. /// //============================================================================= @@ -35,22 +30,29 @@ // Per-bin classification. A bin's kind is which set it lives in (structural), // not a per-bin field. Only Normal feeds coverage(); the rest are recorded. -enum class VlCovBinKind : uint8_t { Normal = 0, Default = 1, Ignore = 2, Illegal = 3 }; +enum class VlCovBinKind : uint8_t { + NORMAL = 0, // Base coverage-collecting bin + DEFAULT = 1, // Bin declared with 'default' range (which is excluded per LRM) + IGNORE = 2, // Ignore bin + ILLEGAL = 3 // Illegal bin +}; //============================================================================= // VlCoverpointIf /// Read-side view of a coverpoint. The writer queries bins by index; the /// implementor computes names/kinds on demand. Bounded bin count, so random -/// access by index is the primary API. +/// access by index is the primary usage. class VlCoverpointIf VL_NOT_FINAL { public: + // CONSTRUCTORS virtual ~VlCoverpointIf() = default; + + // METHODS // All bins, across every set; index range [0, binCount()) virtual int binCount() const = 0; - // Bin name, computed into the caller's reusable buffer (no per-bin alloc after - // warmup); returns buf.c_str() - virtual const char* binName(int i, std::string& buf) const = 0; + // Bin name in declaration order (e.g. "myBin" or "b[3]") + virtual std::string binName(int i) const = 0; virtual VlCovBinKind binKind(int i) const = 0; // Bins covered / effective total (Normal set only) for the coverage calc virtual void coverageParts(double& covered, double& total) const = 0; diff --git a/include/verilated_covergroup.cpp b/include/verilated_covergroup.cpp index ab97aba98..89671831b 100644 --- a/include/verilated_covergroup.cpp +++ b/include/verilated_covergroup.cpp @@ -33,47 +33,46 @@ void VlCoverpoint::init(const char* hier, uint32_t atLeast, int nBins) { void VlCoverpoint::addNamer(VlCovBinKind set, int count, VlCovBinNaming naming, const char* name, const char* file, int line, int col) { - m_namers.push_back(VlCovNamer{set, count, m_nextBase, naming, name, file, line, col}); + m_namers.emplace_back(set, count, m_nextBase, naming, name, file, line, col); m_nextBase += count; - if (set == VlCovBinKind::Normal) m_normal += count; + if (set == VlCovBinKind::NORMAL) m_normal += count; } const VlCovNamer& VlCoverpoint::namerFor(int i) const { // Namers are appended in ascending, contiguous index order covering [0, m_total), // and i is always a valid bin index, so the matching namer always exists. for (const VlCovNamer& nm : m_namers) { - if (i < nm.base + nm.count) return nm; + if (i < nm.base() + nm.count()) return nm; } VL_UNREACHABLE; } -const char* VlCoverpoint::binName(int i, std::string& buf) const { +std::string VlCoverpoint::binName(int i) const { const VlCovNamer& nm = namerFor(i); - buf = nm.name; - if (nm.naming == VlCovBinNaming::Array) buf += '[' + std::to_string(i - nm.base) + ']'; - return buf.c_str(); + std::string name = nm.name(); + if (nm.naming() == VlCovBinNaming::Array) name += '[' + std::to_string(i - nm.base()) + ']'; + return name; } void VlCoverpoint::registerBins(VerilatedCovContext* covcontextp, const char* page) { - std::string buf; for (int i = 0; i < binCount(); ++i) { const VlCovNamer& nm = namerFor(i); const VlCovBinKind kind = binKind(i); - const char* const binp = binName(i, buf); + const std::string binp = binName(i); const std::string full = m_hier + "." + binp; - const std::string lineStr = std::to_string(nm.line); - const std::string colStr = std::to_string(nm.col); - if (kind == VlCovBinKind::Normal) { + const std::string lineStr = std::to_string(nm.line()); + const std::string colStr = std::to_string(nm.col()); + if (kind == VlCovBinKind::NORMAL) { VL_COVER_INSERT(covcontextp, full.c_str(), &m_counts[i], "page", page, "filename", - nm.file, "lineno", lineStr.c_str(), "column", colStr.c_str(), "bin", - binp); + nm.file(), "lineno", lineStr.c_str(), "column", colStr.c_str(), "bin", + binp.c_str()); } else { - const char* const binType = kind == VlCovBinKind::Ignore ? "ignore" - : kind == VlCovBinKind::Illegal ? "illegal" + const char* const binType = kind == VlCovBinKind::IGNORE ? "ignore" + : kind == VlCovBinKind::ILLEGAL ? "illegal" : "default"; VL_COVER_INSERT(covcontextp, full.c_str(), &m_counts[i], "page", page, "filename", - nm.file, "lineno", lineStr.c_str(), "column", colStr.c_str(), "bin", - binp, "bin_type", binType); + nm.file(), "lineno", lineStr.c_str(), "column", colStr.c_str(), "bin", + binp.c_str(), "bin_type", binType); } } } diff --git a/include/verilated_covergroup.h b/include/verilated_covergroup.h index 63a2143ee..85b680e3d 100644 --- a/include/verilated_covergroup.h +++ b/include/verilated_covergroup.h @@ -43,42 +43,70 @@ enum class VlCovBinNaming : uint8_t { Array, // "[i]" bins b[N] value array }; -// Names a contiguous run of bins; carries the run's own source location. +// Specifies the naming scheme for a range of bins, allowing the +// specific name to be computed on-demand. // All name strings are borrowed literals from the generated code. -struct VlCovNamer final { - VlCovBinKind set; // which set the bins belong to - int count; // bins this namer covers (1 for Single) - int base; // first bin index (declaration order), assigned on append - VlCovBinNaming naming; - const char* name; // bin name (Single) or array base name (Array) - const char* file; - int line; - int col; +class VlCovNamer final { + // MEMBERS + VlCovBinKind m_set; // which set the bins belong to + int m_count; // bins this namer covers (1 for Single) + int m_base; // first bin index (declaration order), assigned on append + VlCovBinNaming m_naming; // how bin names are built + const char* m_name; // bin name (Single) or array base name (Array) + const char* m_file; // declaration file + int m_line; // declaration line + int m_col; // declaration column + +public: + // CONSTRUCTORS + VlCovNamer(VlCovBinKind set, int count, int base, VlCovBinNaming naming, const char* name, + const char* file, int line, int col) + : m_set{set} + , m_count{count} + , m_base{base} + , m_naming{naming} + , m_name{name} + , m_file{file} + , m_line{line} + , m_col{col} {} + + // METHODS + VlCovBinKind set() const { return m_set; } + int count() const { return m_count; } + int base() const { return m_base; } + VlCovBinNaming naming() const { return m_naming; } + const char* name() const { return m_name; } + const char* file() const { return m_file; } + int line() const { return m_line; } + int col() const { return m_col; } }; //============================================================================= // VlCoverpoint /// Per-instance coverpoint runtime. Bins are stored in declaration order; a -/// bin's set/name come from the owning namer. coverage() is O(1) via the -/// incrementally-maintained covered count. +/// bin's set/name come from the owning namer. coverage() is computed on demand +/// by scanning bin counts, keeping the sample() hot path a plain counter bump. class VlCoverpoint final : public VlCoverpointIf { + // MEMBERS std::string m_hier; // "covergroup.coverpoint" uint32_t m_atLeast = 1; // option.at_least (coverpoint-wide) int m_total = 0; // bins across all sets int m_normal = 0; // Normal bins (coverage denominator) - int m_numCovered = 0; // Normal bins with count >= m_atLeast int m_nextBase = 0; // running append cursor std::vector m_counts; // [m_total], one per bin std::vector m_namers; // appended in declaration order - const VlCovNamer& namerFor(int i) const; // linear scan (namer count is tiny) + // PRIVATE METHODS + const VlCovNamer& namerFor(int i) const; // obtain the bin-specific name producer void addNamer(VlCovBinKind set, int count, VlCovBinNaming naming, const char* name, const char* file, int line, int col); public: + // CONSTRUCTORS VlCoverpoint() = default; + // METHODS // ---- configuration (from generated constructor) ---- void init(const char* hier, uint32_t atLeast, int nBins); void addSingleNamer(VlCovBinKind set, const char* name, const char* file, int line, int col) { @@ -91,17 +119,24 @@ public: void registerBins(VerilatedCovContext* covcontextp, const char* page); // ---- hot path (from generated sample()) ---- - void incrementBin(int i) { // Normal bin: maintains covered count - if (m_counts[i]++ == m_atLeast - 1) ++m_numCovered; - } + void incrementBin(int i) { ++m_counts[i]; } // Normal bin: count only void recordHit(int i) { ++m_counts[i]; } // Ignore/Illegal/Default: count only // ---- VlCoverpointIf ---- int binCount() const override { return m_total; } - const char* binName(int i, std::string& buf) const override; - VlCovBinKind binKind(int i) const override { return namerFor(i).set; } + std::string binName(int i) const override; + VlCovBinKind binKind(int i) const override { return namerFor(i).set(); } void coverageParts(double& covered, double& total) const override { - covered = m_numCovered; + // Count Normal bins that reached option.at_least on demand, so the hot + // path (incrementBin) stays a plain counter bump. + int numCovered = 0; + for (const VlCovNamer& nm : m_namers) { + if (nm.set() != VlCovBinKind::NORMAL) continue; + for (int i = nm.base(); i < nm.base() + nm.count(); ++i) { + if (m_counts[i] >= m_atLeast) ++numCovered; + } + } + covered = numCovered; total = m_normal; } }; diff --git a/src/V3AstAttr.h b/src/V3AstAttr.h index c12627886..6e4851991 100644 --- a/src/V3AstAttr.h +++ b/src/V3AstAttr.h @@ -1191,6 +1191,19 @@ public: = {"user", "array", "auto", "ignore", "illegal", "default", "wildcard", "transition"}; return names[m_e]; } + // VlCovBinKind enumerator naming the bin's set + const char* binSetEnum() const { + switch (m_e) { + case BINS_IGNORE: return "VlCovBinKind::IGNORE"; + case BINS_ILLEGAL: return "VlCovBinKind::ILLEGAL"; + case BINS_DEFAULT: return "VlCovBinKind::DEFAULT"; + default: return "VlCovBinKind::NORMAL"; + } + } + // Normal bins (feed coverage) are anything but ignore/illegal/default + bool binIsNormal() const { + return m_e != BINS_IGNORE && m_e != BINS_ILLEGAL && m_e != BINS_DEFAULT; + } }; constexpr bool operator==(const VCoverBinsType& lhs, VCoverBinsType::en rhs) { return lhs.m_e == rhs; diff --git a/src/V3Covergroup.cpp b/src/V3Covergroup.cpp index 58dfd47c2..9004926e4 100644 --- a/src/V3Covergroup.cpp +++ b/src/V3Covergroup.cpp @@ -655,22 +655,6 @@ class FunctionalCoverageVisitor final : public VNVisitor { return refp; } - // VlCovBinKind enumerator naming the bin's set - static const char* binSetEnum(AstCoverBin* binp) { - switch (binp->binsType()) { - case VCoverBinsType::BINS_IGNORE: return "VlCovBinKind::Ignore"; - case VCoverBinsType::BINS_ILLEGAL: return "VlCovBinKind::Illegal"; - case VCoverBinsType::BINS_DEFAULT: return "VlCovBinKind::Default"; - default: return "VlCovBinKind::Normal"; - } - } - // Normal bins (feed coverage) are anything but ignore/illegal/default - static bool binIsNormal(AstCoverBin* binp) { - const VCoverBinsType btype = binp->binsType(); - return btype != VCoverBinsType::BINS_IGNORE && btype != VCoverBinsType::BINS_ILLEGAL - && btype != VCoverBinsType::BINS_DEFAULT; - } - // Individual equality targets of an array bin (bins b[N] = {values/ranges}), in order. std::vector extractArrayValues(AstCoverBin* arrayBinp, AstNodeExpr* exprp) { std::vector values; @@ -703,10 +687,11 @@ class FunctionalCoverageVisitor final : public VNVisitor { + std::to_string(fl->lineno()) + ", " + std::to_string(fl->firstColumn()) + ");"; if (count < 0) { // single bin - cs->add(".addSingleNamer(" + std::string{binSetEnum(binp)} + ", \"" + binp->name() + cs->add(".addSingleNamer(" + std::string{binp->binsType().binSetEnum()} + ", \"" + + binp->name() + "\", " + loc); } else { // value array bin - cs->add(".addArrayNamer(" + std::string{binSetEnum(binp)} + ", " + cs->add(".addArrayNamer(" + std::string{binp->binsType().binSetEnum()} + ", " + std::to_string(count) + ", \"" + binp->name() + "\", " + loc); } return cs; @@ -718,7 +703,8 @@ class FunctionalCoverageVisitor final : public VNVisitor { FileLine* const fl = binp->fileline(); AstCStmt* const hitp = new AstCStmt{fl}; hitp->add(memberRef(fl, cpVarp)); - hitp->add((binIsNormal(binp) ? ".incrementBin(" : ".recordHit(") + std::to_string(idx) + hitp->add((binp->binsType().binIsNormal() ? ".incrementBin(" : ".recordHit(") + + std::to_string(idx) + ");"); AstNode* actionp = hitp; if (binp->binsType() == VCoverBinsType::BINS_ILLEGAL) { @@ -1508,7 +1494,7 @@ class FunctionalCoverageVisitor final : public VNVisitor { } int legacyRegular = 0; for (const BinInfo& bi : m_binInfos) { - if (!binIsNormal(bi.binp)) continue; + if (!bi.binp->binsType().binIsNormal()) continue; ++legacyRegular; AstCStmt* const cs = new AstCStmt{fl}; cs->add("if ("); @@ -1532,7 +1518,7 @@ class FunctionalCoverageVisitor final : public VNVisitor { int totalBins = 0; for (const BinInfo& bi : m_binInfos) { UINFO(6, " Bin: " << bi.binp->name() << " type=" << bi.binp->binsType().ascii()); - if (binIsNormal(bi.binp)) totalBins++; + if (bi.binp->binsType().binIsNormal()) totalBins++; } UINFO(4, " Total regular bins: " << totalBins << " of " << m_binInfos.size()); @@ -1560,7 +1546,7 @@ class FunctionalCoverageVisitor final : public VNVisitor { // For each regular bin, if count > 0, increment covered_count for (const BinInfo& bi : m_binInfos) { // Skip ignore/illegal/default bins in coverage calculation - if (!binIsNormal(bi.binp)) continue; + if (!bi.binp->binsType().binIsNormal()) continue; // if (bin_count >= at_least) covered_count++; AstIf* ifp = new AstIf{