From 4351abfe7107487a524fe51b631119487c2fa6b9 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Sun, 25 Apr 2021 11:46:05 +0900 Subject: [PATCH] Fix assertion failure in bitOpTree opt (#2899) * Tests: Add another testcase that triggers assertion failure in bitOpTree opt. * Fix assertion failure in bitOpTree opt reported in #2891. Consider the follwoing case. CCast -> WordSel -> VarRef(leaf) * Make sure that m_bitPolarity is expanded enough. --- src/V3Const.cpp | 33 ++++++++++++++++++++++++------- test_regress/t/t_const_opt_cov.pl | 2 +- test_regress/t/t_const_opt_cov.v | 25 ++++++++++++----------- 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 3d87a2057..a66793857 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -96,11 +96,6 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { ConstBitOpTreeVisitor* m_parentp; // ConstBitOpTreeVisitor that holds this VarInfo AstVarRef* m_refp; // Points the variable that this VarInfo covers V3Number m_bitPolarity; // Coefficient of each bit - static int widthOfRef(AstVarRef* refp) { - if (AstWordSel* selp = VN_CAST(refp->backp(), WordSel)) return selp->width(); - if (AstCCast* castp = VN_CAST(refp->backp(), CCast)) return castp->width(); - return refp->width(); - } public: // METHODS @@ -108,6 +103,30 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { bool sameVarAs(const AstNodeVarRef* otherp) const { return m_refp->sameGateTree(otherp); } void setPolarity(bool compBit, int bit) { UASSERT_OBJ(!hasConstantResult(), m_refp, "Already has result of " << m_constResult); + UASSERT_OBJ(bit < VL_QUADSIZE, m_refp, + "bit:" << bit << " is too big after V3Expand" + << " back:" << m_refp->backp()); + if (bit >= m_bitPolarity.width()) { // Need to expand m_bitPolarity + const V3Number oldPol = std::move(m_bitPolarity); + // oldPol.width() is 8, 16, or 32 because this visitor is called after V3Expand + // newWidth is increased by 2x because + // - CCast will cast to such bitwidth anyway + // - can avoid frequent expansion + int newWidth = oldPol.width(); + while (bit >= newWidth) newWidth *= 2; + m_bitPolarity = V3Number{m_refp, newWidth}; + UASSERT_OBJ(newWidth == 16 || newWidth == 32 || newWidth == 64, m_refp, + "bit:" << bit << " newWidth:" << newWidth); + m_bitPolarity.setAllBitsX(); + for (int i = 0; i < oldPol.width(); ++i) { + if (oldPol.bitIs0(i)) + m_bitPolarity.setBit(i, '0'); + else if (oldPol.bitIs1(i)) + m_bitPolarity.setBit(i, '1'); + } + } + UASSERT_OBJ(bit < m_bitPolarity.width(), m_refp, + "bit:" << bit << " width:" << m_bitPolarity.width() << m_refp); if (m_bitPolarity.bitIsX(bit)) { // The bit is not yet set m_bitPolarity.setBit(bit, compBit); } else { // Priviously set the bit @@ -129,7 +148,7 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { FileLine* fl = m_refp->fileline(); AstNode* srcp = VN_CAST(m_refp->backp(), WordSel); if (!srcp) srcp = m_refp; - const int width = widthOfRef(m_refp); + const int width = m_bitPolarity.width(); if (hasConstantResult()) return new AstConst{fl, @@ -160,7 +179,7 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { VarInfo(ConstBitOpTreeVisitor* parent, AstVarRef* refp) : m_parentp(parent) , m_refp(refp) - , m_bitPolarity(refp, widthOfRef(refp)) { + , m_bitPolarity(refp, refp->isWide() ? VL_EDATASIZE : refp->width()) { m_bitPolarity.setAllBitsX(); } }; diff --git a/test_regress/t/t_const_opt_cov.pl b/test_regress/t/t_const_opt_cov.pl index 1e0a96b74..0a72e4f4f 100755 --- a/test_regress/t/t_const_opt_cov.pl +++ b/test_regress/t/t_const_opt_cov.pl @@ -11,7 +11,7 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(simulator => 1); compile( - verilator_flags2=>["-Wno-UNOPTTHREADS", "--stats", "--coverage"], + verilator_flags2=>["-Wno-UNOPTTHREADS", "--stats", "--coverage", "--trace"], ); execute( diff --git a/test_regress/t/t_const_opt_cov.v b/test_regress/t/t_const_opt_cov.v index 245309007..c939ad806 100644 --- a/test_regress/t/t_const_opt_cov.v +++ b/test_regress/t/t_const_opt_cov.v @@ -20,19 +20,20 @@ module t(/*AUTOARG*/ logic bank_rd_vec_m3; always_ff @(posedge clk) bank_rd_vec_m3 <= crc[33]; - - wire out; - ecc_check_pipe u_bank_data_ecc_check( - .clk (clk), - .bank_rd_m3 (bank_rd_vec_m3), - .data_i (in), - .ecc_err_o (out) - ); - - + logic [3:0][31:0] data_i; + wire [3:0] out; + for (genvar i = 0; i < 4; ++i) begin + always_ff @(posedge clk) data_i[i] <= crc[63:32]; + ecc_check_pipe u_bank_data_ecc_check( + .clk (clk), + .bank_rd_m3 (bank_rd_vec_m3), + .data_i ({1'b0, data_i[i]}), + .ecc_err_o (out[i]) + ); + end // Aggregate outputs into a single result vector - wire [63:0] result = {63'b0, out}; + wire [63:0] result = {60'b0, out}; // Test loop always @ (posedge clk) begin @@ -54,7 +55,7 @@ module t(/*AUTOARG*/ $write("[%0t] cyc==%0d crc=%x sum=%x\n",$time, cyc, crc, sum); if (crc !== 64'hc77bb9b3784ea091) $stop; // What checksum will we end up with (above print should match) -`define EXPECTED_SUM 64'h768b162c5835e35b +`define EXPECTED_SUM 64'ha2601675a6ae4972 if (sum !== `EXPECTED_SUM) $stop; $write("*-* All Finished *-*\n"); $finish;