From d6c2e2faf622649755fc80cc96c3b4a9c973c9bd Mon Sep 17 00:00:00 2001 From: Rupert Swarbrick Date: Mon, 29 Mar 2021 00:57:36 +0100 Subject: [PATCH] Allow overlaps in priority case statements (#2864) This will still warn if a case item is completely covered by previous items, but will no longer complain about overlaps like this: priority casez (foo_i) 2'b ?1: bar_o = 3'd0; 2'b 1?: bar_o = 3'd1; Before, there was a warning for the second statement because the first two patterns match 2'b11. --- src/V3Case.cpp | 47 +++++++++++++++++++++------- test_regress/t/t_priority_case.out | 8 +++++ test_regress/t/t_priority_case.pl | 19 ++++++++++++ test_regress/t/t_priority_case.v | 50 ++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 11 deletions(-) create mode 100644 test_regress/t/t_priority_case.out create mode 100755 test_regress/t/t_priority_case.pl create mode 100644 test_regress/t/t_priority_case.v diff --git a/src/V3Case.cpp b/src/V3Case.cpp index 1b3122d7d..23653a592 100644 --- a/src/V3Case.cpp +++ b/src/V3Case.cpp @@ -159,11 +159,13 @@ private: return false; // Too wide for analysis } UINFO(8, "Simple case statement: " << nodep << endl); + uint32_t numCases = 1UL << m_caseWidth; // Zero list of items for each value - for (uint32_t i = 0; i < (1UL << m_caseWidth); ++i) m_valueItem[i] = nullptr; + for (uint32_t i = 0; i < numCases; ++i) m_valueItem[i] = nullptr; // Now pick up the values for each assignment // We can cheat and use uint32_t's because we only support narrow case's - bool bitched = false; + bool reportedOverlap = false; + bool reportedSubcase = false; for (AstCaseItem* itemp = nodep->itemsp(); itemp; itemp = VN_CAST(itemp->nextp(), CaseItem)) { for (AstNode* icondp = itemp->condsp(); icondp; icondp = icondp->nextp()) { @@ -179,29 +181,52 @@ private: V3Number numval(itemp, iconstp->width()); numval.opBitsOne(iconstp->num()); uint32_t val = numval.toUInt(); - for (uint32_t i = 0; i < (1UL << m_caseWidth); ++i) { + + uint32_t firstOverlap = 0; + bool foundOverlap = false; + bool foundHit = false; + for (uint32_t i = 0; i < numCases; ++i) { if ((i & mask) == val) { if (!m_valueItem[i]) { m_valueItem[i] = itemp; - } else if (!itemp->ignoreOverlap() && !bitched) { - icondp->v3warn(CASEOVERLAP, - "Case values overlap (example pattern 0x" - << std::hex << i << ")"); - bitched = true; + foundHit = true; + } else if (!foundOverlap && !itemp->ignoreOverlap()) { + firstOverlap = i; + foundOverlap = true; m_caseNoOverlapsAllCovered = false; } } } + if (!nodep->priorityPragma()) { + // If this case statement doesn't have the priority + // keyword, we want to warn on any overlap. + if (!reportedOverlap && foundOverlap) { + icondp->v3warn(CASEOVERLAP, "Case values overlap (example pattern 0x" + << std::hex << firstOverlap << ")"); + reportedOverlap = true; + } + } else { + // If this is a priority case, we only want to complain + // if every possible value for this item is already hit + // by some other item. This is true if foundHit is + // false. + if (!reportedSubcase && !foundHit) { + icondp->v3warn(CASEOVERLAP, + "Case item ignored: every matching value is covered " + "by an earlier item"); + reportedSubcase = true; + } + } } } // Defaults were moved to last in the caseitem list by V3LinkDot if (itemp->isDefault()) { // Case statement's default... Fill the table - for (uint32_t i = 0; i < (1UL << m_caseWidth); ++i) { + for (uint32_t i = 0; i < numCases; ++i) { if (!m_valueItem[i]) m_valueItem[i] = itemp; } } } - for (uint32_t i = 0; i < (1UL << m_caseWidth); ++i) { + for (uint32_t i = 0; i < numCases; ++i) { if (!m_valueItem[i]) { nodep->v3warn(CASEINCOMPLETE, "Case values incompletely covered " "(example pattern 0x" @@ -218,7 +243,7 @@ private: // Convert valueItem from AstCaseItem* to the expression // Not done earlier, as we may now have a nullptr because it's just a ";" NOP branch - for (uint32_t i = 0; i < (1UL << m_caseWidth); ++i) { + for (uint32_t i = 0; i < numCases; ++i) { m_valueItem[i] = VN_CAST(m_valueItem[i], CaseItem)->bodysp(); } return true; // All is fine diff --git a/test_regress/t/t_priority_case.out b/test_regress/t/t_priority_case.out new file mode 100644 index 000000000..f3e336f9d --- /dev/null +++ b/test_regress/t/t_priority_case.out @@ -0,0 +1,8 @@ +%Warning-CASEOVERLAP: t/t_priority_case.v:34:7: Case item ignored: every matching value is covered by an earlier item + 34 | 2'b ?1: out1 = 3'd1; + | ^~~~~~ + ... Use "/* verilator lint_off CASEOVERLAP */" and lint_on around source to disable this message. +%Warning-CASEOVERLAP: t/t_priority_case.v:44:7: Case item ignored: every matching value is covered by an earlier item + 44 | 2'b ?1: out1 = 3'd1; + | ^~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_priority_case.pl b/test_regress/t/t_priority_case.pl new file mode 100755 index 000000000..4f29861db --- /dev/null +++ b/test_regress/t/t_priority_case.pl @@ -0,0 +1,19 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2021 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(linter => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_priority_case.v b/test_regress/t/t_priority_case.v new file mode 100644 index 000000000..477752748 --- /dev/null +++ b/test_regress/t/t_priority_case.v @@ -0,0 +1,50 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2021 by Rupert Swarbrick. +// SPDX-License-Identifier: CC0-1.0 + +module t(/*AUTOARG*/ + // Inputs + value + ); + input [1:0] value; + + sub u_sub(.value(value), .out0(), .out1()); + +endmodule + +module sub (input logic [1:0] value, + output logic [2:0] out0, + output logic [2:0] out1); + + always_comb begin + // This case statement shouldn't cause any warnings. Although the cases overlap (2'b11 matches + // both 2'b?1 and 2'b1?), the second item matches 2'b10 and the first one doesn't. + priority casez (value) + 2'b ?1: out0 = 3'd0; + 2'b 1?: out0 = 3'd1; + default: out0 = 3'd2; + endcase + + // This case statement *should* cause a warning: the second case is completely covered by the + // first. + priority casez (value) + 2'b ?1: out1 = 3'd0; + 2'b ?1: out1 = 3'd1; + default: out1 = 3'd2; + endcase + + // This case statement should cause a warning too: the second case and third cases are + // completely covered by the first. But it should only cause one: like with overlapping cases, + // we assume that the author messed up the first case, so there's no real benefit to reporting + // each thing it subsumes. + priority casez (value) + 2'b ?1: out1 = 3'd0; + 2'b ?1: out1 = 3'd1; + 2'b 11: out1 = 3'd2; + default: out1 = 3'd3; + endcase + end + +endmodule