From aa8e7f00cd053d3e3da9de211ee2135d5e415880 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Thu, 3 Aug 2023 16:32:03 +0200 Subject: [PATCH 01/27] VlEvent -> VlEventHandle - use shared_ptr for events Signed-off-by: Krzysztof Boronski --- include/verilated_fst_c.cpp | 2 +- include/verilated_fst_c.h | 2 +- include/verilated_trace.h | 8 +++++--- include/verilated_trace_imp.h | 4 ++-- include/verilated_types.h | 27 ++++++++++++++++++++------- include/verilated_vcd_c.cpp | 2 +- include/verilated_vcd_c.h | 2 +- src/V3AstNodes.cpp | 2 +- src/V3EmitCSyms.cpp | 8 ++++---- 9 files changed, 36 insertions(+), 21 deletions(-) diff --git a/include/verilated_fst_c.cpp b/include/verilated_fst_c.cpp index ed93bdf64b..b3dc26c8dc 100644 --- a/include/verilated_fst_c.cpp +++ b/include/verilated_fst_c.cpp @@ -284,7 +284,7 @@ void VerilatedFst::configure(const VerilatedTraceConfig& config) { // so always inline them. VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitEvent(uint32_t code, VlEvent newval) { +void VerilatedFstBuffer::emitEvent(uint32_t code, VlEventHandle newval) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); fstWriterEmitValueChange(m_fst, m_symbolp[code], "1"); } diff --git a/include/verilated_fst_c.h b/include/verilated_fst_c.h index 57671e0d70..c3a6eacbdd 100644 --- a/include/verilated_fst_c.h +++ b/include/verilated_fst_c.h @@ -163,7 +163,7 @@ class VerilatedFstBuffer VL_NOT_FINAL { // Implementations of duck-typed methods for VerilatedTraceBuffer. These are // called from only one place (the full* methods), so always inline them. - VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEvent newval); + VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEventHandle newval); VL_ATTR_ALWINLINE void emitBit(uint32_t code, CData newval); VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int bits); VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int bits); diff --git a/include/verilated_trace.h b/include/verilated_trace.h index 7e8694b705..4a969ded26 100644 --- a/include/verilated_trace.h +++ b/include/verilated_trace.h @@ -423,7 +423,7 @@ class VerilatedTraceBuffer VL_NOT_FINAL : public T_Buffer { void fullQData(uint32_t* oldp, QData newval, int bits); void fullWData(uint32_t* oldp, const WData* newvalp, int bits); void fullDouble(uint32_t* oldp, double newval); - void fullEvent(uint32_t* oldp, VlEvent newval); + void fullEvent(uint32_t* oldp, VlEventHandle newval); // In non-offload mode, these are called directly by the trace callbacks, // and are called chg*. In offload mode, they are called by the worker @@ -460,7 +460,9 @@ class VerilatedTraceBuffer VL_NOT_FINAL : public T_Buffer { } } } - VL_ATTR_ALWINLINE void chgEvent(uint32_t* oldp, VlEvent newval) { fullEvent(oldp, newval); } + VL_ATTR_ALWINLINE void chgEvent(uint32_t* oldp, VlEventHandle newval) { + fullEvent(oldp, newval); + } VL_ATTR_ALWINLINE void chgDouble(uint32_t* oldp, double newval) { double old; std::memcpy(&old, oldp, sizeof(old)); @@ -539,7 +541,7 @@ class VerilatedTraceOffloadBuffer final : public VerilatedTraceBuffer m_offloadBufferWritep += 4; VL_DEBUG_IF(assert(m_offloadBufferWritep <= m_offloadBufferEndp);); } - void chgEvent(uint32_t code, VlEvent newval) { + void chgEvent(uint32_t code, VlEventHandle newval) { m_offloadBufferWritep[0] = VerilatedTraceOffloadCommand::CHG_EVENT; m_offloadBufferWritep[1] = code; m_offloadBufferWritep += 2; diff --git a/include/verilated_trace_imp.h b/include/verilated_trace_imp.h index 9f57921271..5a16a1176e 100644 --- a/include/verilated_trace_imp.h +++ b/include/verilated_trace_imp.h @@ -185,7 +185,7 @@ void VerilatedTrace::offloadWorkerThreadMain() { continue; case VerilatedTraceOffloadCommand::CHG_EVENT: VL_TRACE_OFFLOAD_DEBUG("Command CHG_EVENT " << top); - traceBufp->chgEvent(oldp, *reinterpret_cast(readp)); + traceBufp->chgEvent(oldp, *reinterpret_cast(readp)); continue; //=== @@ -839,7 +839,7 @@ void VerilatedTraceBuffer::fullBit(uint32_t* oldp, CData newval) { } template <> -void VerilatedTraceBuffer::fullEvent(uint32_t* oldp, VlEvent newval) { +void VerilatedTraceBuffer::fullEvent(uint32_t* oldp, VlEventHandle newval) { const uint32_t code = oldp - m_sigs_oldvalp; *oldp = 1; // Do we really store an "event" ? emitEvent(code, newval); diff --git a/include/verilated_types.h b/include/verilated_types.h index 9eae185d56..7ac1ba9fea 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -172,18 +173,30 @@ class VlEvent final { VlEvent() = default; ~VlEvent() = default; - // METHODS - void fire() { m_fired = m_triggered = true; } - bool isFired() const { return m_fired; } - bool isTriggered() const { return m_triggered; } - void clearFired() { m_fired = false; } - void clearTriggered() { m_triggered = false; } + friend std::string VL_TO_STRING(const VlEvent& e); + friend class VlEventHandle; }; inline std::string VL_TO_STRING(const VlEvent& e) { - return std::string{"triggered="} + (e.isTriggered() ? "true" : "false"); + return std::string{"triggered="} + (e.m_triggered ? "true" : "false"); } +class VlEventHandle : public std::shared_ptr { +public: + // Constructor + VlEventHandle() + : std::shared_ptr(new VlEvent) {} + + // METHODS + void fire() { (*this)->m_fired = (*this)->m_triggered = true; } + bool isFired() const { return (*this)->m_fired; } + bool isTriggered() const { return (*this)->m_triggered; } + void clearFired() { (*this)->m_fired = false; } + void clearTriggered() { (*this)->m_triggered = false; } +}; + +inline std::string VL_TO_STRING(const VlEventHandle& e) { return "&{ " + VL_TO_STRING(*e) + " }"; } + //=================================================================== // Random diff --git a/include/verilated_vcd_c.cpp b/include/verilated_vcd_c.cpp index b05967c1b5..b57babeaa9 100644 --- a/include/verilated_vcd_c.cpp +++ b/include/verilated_vcd_c.cpp @@ -669,7 +669,7 @@ void VerilatedVcdBuffer::finishLine(uint32_t code, char* writep) { // so always inline them. VL_ATTR_ALWINLINE -void VerilatedVcdBuffer::emitEvent(uint32_t code, VlEvent newval) { +void VerilatedVcdBuffer::emitEvent(uint32_t code, VlEventHandle newval) { const bool triggered = newval.isTriggered(); // TODO : It seems that untriggered events are not filtered // should be tested before this last step diff --git a/include/verilated_vcd_c.h b/include/verilated_vcd_c.h index 9ffceaa876..d806f6214a 100644 --- a/include/verilated_vcd_c.h +++ b/include/verilated_vcd_c.h @@ -207,7 +207,7 @@ class VerilatedVcdBuffer VL_NOT_FINAL { // Implementation of VerilatedTraceBuffer interface // Implementations of duck-typed methods for VerilatedTraceBuffer. These are // called from only one place (the full* methods), so always inline them. - VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEvent newval); + VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEventHandle newval); VL_ATTR_ALWINLINE void emitBit(uint32_t code, CData newval); VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int bits); VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int bits); diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 507bcfbbc7..7f129df82c 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -858,7 +858,7 @@ AstNodeDType::CTypeRecursed AstNodeDType::cTypeRecurse(bool compound) const { } else if (bdtypep->isProcessRef()) { info.m_type = "VlProcessRef"; } else if (bdtypep->isEvent()) { - info.m_type = "VlEvent"; + info.m_type = "VlEventHandle"; } else if (dtypep->widthMin() <= 8) { // Handle unpacked arrays; not bdtypep->width info.m_type = "CData" + bitvec; } else if (dtypep->widthMin() <= 16) { diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index 4a041d784d..05f69166c5 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -464,7 +464,7 @@ void EmitCSyms::emitSymHdr() { puts("uint32_t __Vm_baseCode = 0;" " ///< Used by trace routines when tracing multiple models\n"); } - if (v3Global.hasEvents()) puts("std::vector __Vm_triggeredEvents;\n"); + if (v3Global.hasEvents()) puts("std::vector __Vm_triggeredEvents;\n"); if (v3Global.hasClasses()) puts("VlDeleter __Vm_deleter;\n"); puts("bool __Vm_didInit = false;\n"); @@ -532,17 +532,17 @@ void EmitCSyms::emitSymHdr() { puts("const char* name() { return TOP.name(); }\n"); if (v3Global.hasEvents()) { - puts("void enqueueTriggeredEventForClearing(VlEvent& event) {\n"); + puts("void enqueueTriggeredEventForClearing(VlEventHandle& event) {\n"); puts("#ifdef VL_DEBUG\n"); puts("if (VL_UNLIKELY(!event.isTriggered())) {\n"); puts("VL_FATAL_MT(__FILE__, __LINE__, __FILE__, \"event passed to " "'enqueueTriggeredEventForClearing' was not triggered\");\n"); puts("}\n"); puts("#endif\n"); - puts("__Vm_triggeredEvents.push_back(&event);\n"); + puts("__Vm_triggeredEvents.push_back(event);\n"); puts("}\n"); puts("void clearTriggeredEvents() {\n"); - puts("for (const auto eventp : __Vm_triggeredEvents) eventp->clearTriggered();\n"); + puts("for (auto& eventp : __Vm_triggeredEvents) eventp.clearTriggered();\n"); puts("__Vm_triggeredEvents.clear();\n"); puts("}\n"); } From 20ad9e157dd57adda18397e0f3dedf51a17943c0 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Thu, 3 Aug 2023 19:30:05 +0200 Subject: [PATCH 02/27] Allow event assignement, allow firing events from expressions, but throw a warning when doing so Signed-off-by: Krzysztof Boronski --- src/V3Error.h | 5 +++-- src/V3LinkDot.cpp | 7 +++++++ src/V3Width.cpp | 14 ++++++++------ src/verilog.y | 4 ++-- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/V3Error.h b/src/V3Error.h index 87c6c0badc..28e2804f55 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -98,6 +98,7 @@ class V3ErrorCode final { ENDLABEL, // End lable name mismatch ENUMVALUE, // Error: enum type needs explicit cast EOFNEWLINE, // End-of-file missing newline + EVENTEXPR, // Non-identifier expression used to send an event GENCLK, // Generated Clock. Historical, never issued. GENUNNAMED, // Generate unnamed, without label HIERBLOCK, // Ignored hierarchical block setting @@ -194,8 +195,8 @@ class V3ErrorCode final { "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", "CDCRSTLOGIC", "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", "CONTASSREG", "DECLFILENAME", "DEFPARAM", "DEPRECATED", - "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "GENUNNAMED", - "HIERBLOCK", + "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "EVENTEXPR", "GENCLK", + "GENUNNAMED", "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE", diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 7ea95b46f6..06333b0a5a 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -3381,6 +3381,13 @@ class LinkDotResolveVisitor final : public VNVisitor { } m_ds.m_dotSymp = m_curSymp = oldCurSymp; } + void visit(AstFireEvent* nodep) override { + visit(static_cast(nodep)); + if (!VN_IS(nodep->operandp(), VarRef)) { + nodep->v3warn(EVENTEXPR, "Non-identifier expression used to reference an event to be " + "sent. This is not a part of IEEE_1800-2017"); + } + } void visit(AstWith* nodep) override { UINFO(5, " " << nodep << endl); checkNoDot(nodep); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index cecd6ad1ba..1a9a5de968 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4700,12 +4700,14 @@ class WidthVisitor final : public VNVisitor { iterateCheckAssign(nodep, "Assign RHS", nodep->rhsp(), FINAL, lhsDTypep); // if (debug()) nodep->dumpTree("- AssignOut: "); } - if (const AstBasicDType* const basicp = nodep->rhsp()->dtypep()->basicp()) { - if (basicp->isEvent()) { - // see t_event_copy.v for commentary on the mess involved - nodep->v3warn(E_UNSUPPORTED, "Unsupported: assignment of event data type"); - } - } + // FIXME: This should still report an error in cases where events would be handled + // by static scheduler (__VtrigSched), ie. evaluated in active loop. + //if (const AstBasicDType* const basicp = nodep->rhsp()->dtypep()->basicp()) { + // if (basicp->isEvent()) { + // // see t_event_copy.v for commentary on the mess involved + // nodep->v3warn(E_UNSUPPORTED, "Unsupported: assignment of event data type"); + // } + //} if (auto* const controlp = nodep->timingControlp()) { if (VN_IS(m_ftaskp, Func)) { controlp->v3error("Timing controls are not legal in functions. Suggest use a task " diff --git a/src/verilog.y b/src/verilog.y index cfc01f62af..d166be17ec 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3584,9 +3584,9 @@ statement_item: // IEEE: statement_item | yDISABLE idAny '.' idDotted ';' { $$ = nullptr; BBUNSUP($4, "Unsupported: disable with '.'"); } // // IEEE: event_trigger - | yP_MINUSGT idDotted/*hierarchical_identifier-event*/ ';' + | yP_MINUSGT expr ';' { $$ = new AstFireEvent{$1, $2, false}; } - | yP_MINUSGTGT delay_or_event_controlE idDotted/*hierarchical_identifier-event*/ ';' + | yP_MINUSGTGT delay_or_event_controlE expr ';' { $$ = new AstFireEvent{$1, $3, true}; } // // // IEEE: loop_statement From b68c213c29ea6cdde7a7811f94ffd0ecc2dc5737 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Thu, 3 Aug 2023 20:31:25 +0200 Subject: [PATCH 03/27] Add regression tests Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_assign.pl | 23 ++++++++ test_regress/t/t_event_control_assign.v | 68 ++++++++++++++++++++++++ test_regress/t/t_event_control_pass.pl | 23 ++++++++ test_regress/t/t_event_control_pass.v | 54 +++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100755 test_regress/t/t_event_control_assign.pl create mode 100644 test_regress/t/t_event_control_assign.v create mode 100755 test_regress/t/t_event_control_pass.pl create mode 100644 test_regress/t/t_event_control_pass.v diff --git a/test_regress/t/t_event_control_assign.pl b/test_regress/t/t_event_control_assign.pl new file mode 100755 index 0000000000..00a80e5f2a --- /dev/null +++ b/test_regress/t/t_event_control_assign.pl @@ -0,0 +1,23 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2023 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(simulator => 1); + +compile( + verilator_flags2 => ["--exe --main --timing"], + make_main => 0, + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_event_control_assign.v b/test_regress/t/t_event_control_assign.v new file mode 100644 index 0000000000..980bd9ef88 --- /dev/null +++ b/test_regress/t/t_event_control_assign.v @@ -0,0 +1,68 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + + +int evt_recv_cnt; +int new_evt_recv_cnt; + +module t(); + + class Foo; + event evt1; + + task automatic send_evt(); + fork + #10 begin ->evt1; end + begin + event new_event; + #20; + // This should cause an event merge but for now we don't support that. + evt1 = new_event; + end + #30 begin + @evt1 $display("Received a new event"); + new_evt_recv_cnt++; + end + join_none + endtask + + task wait_for_event(); + fork begin + @evt1 $display("Received evt1"); + evt_recv_cnt++; + end join_none + endtask + + endclass + + initial begin + Foo foo1; + foo1 = new; + + evt_recv_cnt = 0; + new_evt_recv_cnt = 0; + + for (int i = 0; i < 4; i++) begin + foo1.wait_for_event(); + #10; + foo1.send_evt(); + #90; + $display("- end of iteration -"); + if (evt_recv_cnt != i + 1) + $stop; + if (new_evt_recv_cnt != i) + $stop; + end + + if (evt_recv_cnt != 4) + $stop; + if (new_evt_recv_cnt != 3) + $stop; + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_event_control_pass.pl b/test_regress/t/t_event_control_pass.pl new file mode 100755 index 0000000000..9d09ae0b12 --- /dev/null +++ b/test_regress/t/t_event_control_pass.pl @@ -0,0 +1,23 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2023 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(simulator => 1); + +compile( + verilator_flags2 => ["--exe --main --timing -Wno-EVENTEXPR"], + make_main => 0, + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_event_control_pass.v b/test_regress/t/t_event_control_pass.v new file mode 100644 index 0000000000..17aebd6f96 --- /dev/null +++ b/test_regress/t/t_event_control_pass.v @@ -0,0 +1,54 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +class Bar; + event evt; + + task automatic wait_for_event(); + @evt; + endtask + + function automatic event get_event(); + return evt; + endfunction +endclass + +class Foo; + task automatic send_event(Bar b); + ->b.get_event(); + endtask +endclass + +bit got_event; + +module t(); + + initial begin + Bar bar; + Foo foo; + foo = new; + bar = new; + got_event = 0; + + fork + begin + bar.wait_for_event(); + $display("Got the event!"); + got_event = 1; + end + #10 foo.send_event(bar); + join + + #99; + if (!got_event) + $stop; + $write("*-* All Finished *-*\n"); + $finish; + end + + initial begin + end +endmodule From 75a0630ca1403985bcdbaa6b9f1cf04dc9ab47f0 Mon Sep 17 00:00:00 2001 From: github action Date: Thu, 3 Aug 2023 18:38:57 +0000 Subject: [PATCH 04/27] Apply 'make format' --- src/V3Width.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 1a9a5de968..a7d7ef894a 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4702,7 +4702,7 @@ class WidthVisitor final : public VNVisitor { } // FIXME: This should still report an error in cases where events would be handled // by static scheduler (__VtrigSched), ie. evaluated in active loop. - //if (const AstBasicDType* const basicp = nodep->rhsp()->dtypep()->basicp()) { + // if (const AstBasicDType* const basicp = nodep->rhsp()->dtypep()->basicp()) { // if (basicp->isEvent()) { // // see t_event_copy.v for commentary on the mess involved // nodep->v3warn(E_UNSUPPORTED, "Unsupported: assignment of event data type"); From 319aaaab6b52c7cf1dc0dfe8aa16b6343b8f0521 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Fri, 4 Aug 2023 16:19:46 +0200 Subject: [PATCH 05/27] Fix EVENTEXPR check Signed-off-by: Krzysztof Boronski --- src/V3LinkDot.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 06333b0a5a..5db83da293 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -3383,9 +3383,11 @@ class LinkDotResolveVisitor final : public VNVisitor { } void visit(AstFireEvent* nodep) override { visit(static_cast(nodep)); - if (!VN_IS(nodep->operandp(), VarRef)) { - nodep->v3warn(EVENTEXPR, "Non-identifier expression used to reference an event to be " - "sent. This is not a part of IEEE_1800-2017"); + const AstNodeExpr* operandp = nodep->operandp(); + while (const AstMemberSel* mselp = VN_CAST(operandp, MemberSel)) operandp = mselp->fromp(); + if (!VN_IS(operandp, VarRef)) { + operandp->v3warn(EVENTEXPR, "Non-identifier expression used to reference an event to " + "be sent. This is not a part of IEEE_1800-2017"); } } void visit(AstWith* nodep) override { From 980c3348c6812c407f762d567cd7748b21a23b88 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Fri, 4 Aug 2023 17:17:07 +0200 Subject: [PATCH 06/27] Add regression test for EVENTEXPR warning Signed-off-by: Krzysztof Boronski --- .../t/t_event_control_eventexpr_bad.out | 7 ++++++ .../t/t_event_control_eventexpr_bad.pl | 22 +++++++++++++++++++ .../t/t_event_control_eventexpr_bad.v | 15 +++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 test_regress/t/t_event_control_eventexpr_bad.out create mode 100755 test_regress/t/t_event_control_eventexpr_bad.pl create mode 100644 test_regress/t/t_event_control_eventexpr_bad.v diff --git a/test_regress/t/t_event_control_eventexpr_bad.out b/test_regress/t/t_event_control_eventexpr_bad.out new file mode 100644 index 0000000000..5e0131123f --- /dev/null +++ b/test_regress/t/t_event_control_eventexpr_bad.out @@ -0,0 +1,7 @@ +%Warning-WIDTHTRUNC: t/t_flag_werror.v:10:19: Operator ASSIGNW expects 4 bits on the Assign RHS, but Assign RHS's CONST '6'h2e' generates 6 bits. + : ... In instance t + 10 | wire [3:0] foo = 6'h2e; + | ^ + ... For warning description see https://verilator.org/warn/WIDTHTRUNC?v=latest + ... Use "/* verilator lint_off WIDTHTRUNC */" and lint_on around source to disable this message. +%Error: Exiting due to diff --git a/test_regress/t/t_event_control_eventexpr_bad.pl b/test_regress/t/t_event_control_eventexpr_bad.pl new file mode 100755 index 0000000000..364abc1a10 --- /dev/null +++ b/test_regress/t/t_event_control_eventexpr_bad.pl @@ -0,0 +1,22 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2023 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); + +top_filename("t/t_flag_werror.v"); + +lint( + verilator_flags2 => ["--timing"], + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_event_control_eventexpr_bad.v b/test_regress/t/t_event_control_eventexpr_bad.v new file mode 100644 index 0000000000..a2e544ecaf --- /dev/null +++ b/test_regress/t/t_event_control_eventexpr_bad.v @@ -0,0 +1,15 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +class Foo; + event evt; + function automatic event get_event(); + return evt; + endfunction + task send_event(); + ->get_event(); + endtask +endclass From 1cb041dab07effac2ad317a46e358bbb95ada36f Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Fri, 4 Aug 2023 18:08:37 +0200 Subject: [PATCH 07/27] make VlEventHandle final Signed-off-by: Krzysztof Boronski --- include/verilated_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/verilated_types.h b/include/verilated_types.h index 7ac1ba9fea..7dc4b4c3ec 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -181,7 +181,7 @@ inline std::string VL_TO_STRING(const VlEvent& e) { return std::string{"triggered="} + (e.m_triggered ? "true" : "false"); } -class VlEventHandle : public std::shared_ptr { +class VlEventHandle final : public std::shared_ptr { public: // Constructor VlEventHandle() From 1b3408309ce82bec534661a3372a8efff956ab62 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 7 Aug 2023 16:39:21 +0200 Subject: [PATCH 08/27] Disallow event assignements in statically scheduled context Signed-off-by: Krzysztof Boronski --- src/V3Ast.h | 1 + src/V3AstInlines.h | 4 ++++ src/V3Width.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/src/V3Ast.h b/src/V3Ast.h index 38dba77144..71a9399979 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1879,6 +1879,7 @@ class AstNode VL_NOT_FINAL { inline bool isDouble() const VL_MT_STABLE; inline bool isSigned() const VL_MT_STABLE; inline bool isString() const VL_MT_STABLE; + inline bool isEvent() const VL_MT_STABLE; // clang-format off VNUser user1u() const VL_MT_STABLE { diff --git a/src/V3AstInlines.h b/src/V3AstInlines.h index 3c40963b28..f47aa7be8b 100644 --- a/src/V3AstInlines.h +++ b/src/V3AstInlines.h @@ -39,6 +39,10 @@ bool AstNode::isDouble() const VL_MT_STABLE { bool AstNode::isString() const VL_MT_STABLE { return dtypep() && dtypep()->basicp() && dtypep()->basicp()->isString(); } +bool AstNode::isEvent() const VL_MT_STABLE { + return dtypep() && dtypep()->basicp() && dtypep()->basicp()->isEvent(); +} + bool AstNode::isSigned() const VL_MT_STABLE { return dtypep() && dtypep()->isSigned(); } bool AstNode::isClassHandleValue() const { diff --git a/src/V3Width.cpp b/src/V3Width.cpp index a7d7ef894a..804d6cb827 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -71,6 +71,7 @@ #include "V3Width.h" #include "V3Const.h" +#include "V3Error.h" #include "V3Global.h" #include "V3MemberMap.h" #include "V3Number.h" @@ -4306,6 +4307,46 @@ class WidthVisitor final : public VNVisitor { return valuep; } + static void checkEventAssignement(const AstNodeAssign* const asgnp) { + string unsupEvtAsgn; + if (!usesDynamicScheduler(asgnp->lhsp())) unsupEvtAsgn = "to"; + if (asgnp->rhsp()->dtypep()->isEvent() && !usesDynamicScheduler(asgnp->rhsp())) { + unsupEvtAsgn += (unsupEvtAsgn.empty() ? "from" : " and from"); + } + if (!unsupEvtAsgn.empty()) { + asgnp->v3warn(E_UNSUPPORTED, "Assignement " + << unsupEvtAsgn + << " event in statically scheduled context.\n" + << asgnp->warnMore() + << "Static event " + "scheduling won't be able to handle this. " + "Your best bet is to move the event into a " + "completely dynamic context, eg. a class, and " + "reference it only from such context."); + } + } + + static bool usesDynamicScheduler(AstNode* nodep) { + UASSERT_OBJ(nodep->dtypep()->isEvent(), nodep, "Node does not have an event dtype"); + + AstVarRef* vrefp; + while (true) { + vrefp = VN_CAST(nodep, VarRef); + if (vrefp) return usesDynamicScheduler(vrefp); + if (VN_IS(nodep, MemberSel)) { + return true; + } else if (AstNodeSel* selp = VN_CAST(nodep, NodeSel)) { + nodep = selp->fromp(); + } else { + return false; + } + } + } + + static bool usesDynamicScheduler(AstVarRef* vrefp) { + return VN_IS(vrefp->classOrPackagep(), Class) || vrefp->varp()->isFuncLocal(); + } + void visit(AstPatMember* nodep) override { AstNodeDType* const vdtypep = m_vup->dtypeNullp(); UASSERT_OBJ(vdtypep, nodep, "Pattern member type not assigned by AstPattern visitor"); @@ -4764,6 +4805,9 @@ class WidthVisitor final : public VNVisitor { VL_DO_DANGLING(pushDeletep(nodep), nodep); // return; } + + if (nodep->dtypep()->isEvent()) + checkEventAssignement(nodep); } void visit(AstRelease* nodep) override { From e41bebe49dce15a664114444397f3ce2ca36806f Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 7 Aug 2023 16:39:46 +0200 Subject: [PATCH 09/27] Update t_event_copy test Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_eventexpr_bad.out | 11 +++++------ test_regress/t/t_event_control_eventexpr_bad.pl | 2 -- test_regress/t/t_event_copy.out | 11 +++++++++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/test_regress/t/t_event_control_eventexpr_bad.out b/test_regress/t/t_event_control_eventexpr_bad.out index 5e0131123f..c83d3afd31 100644 --- a/test_regress/t/t_event_control_eventexpr_bad.out +++ b/test_regress/t/t_event_control_eventexpr_bad.out @@ -1,7 +1,6 @@ -%Warning-WIDTHTRUNC: t/t_flag_werror.v:10:19: Operator ASSIGNW expects 4 bits on the Assign RHS, but Assign RHS's CONST '6'h2e' generates 6 bits. - : ... In instance t - 10 | wire [3:0] foo = 6'h2e; - | ^ - ... For warning description see https://verilator.org/warn/WIDTHTRUNC?v=latest - ... Use "/* verilator lint_off WIDTHTRUNC */" and lint_on around source to disable this message. +%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:13:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 + 13 | ->get_event(); + | ^~~~~~~~~ + ... For warning description see https://verilator.org/warn/EVENTEXPR?v=latest + ... Use "/* verilator lint_off EVENTEXPR */" and lint_on around source to disable this message. %Error: Exiting due to diff --git a/test_regress/t/t_event_control_eventexpr_bad.pl b/test_regress/t/t_event_control_eventexpr_bad.pl index 364abc1a10..54bfd3960a 100755 --- a/test_regress/t/t_event_control_eventexpr_bad.pl +++ b/test_regress/t/t_event_control_eventexpr_bad.pl @@ -10,8 +10,6 @@ scenarios(linter => 1); -top_filename("t/t_flag_werror.v"); - lint( verilator_flags2 => ["--timing"], fails => 1, diff --git a/test_regress/t/t_event_copy.out b/test_regress/t/t_event_copy.out index 6be6494fe6..f802a38b54 100644 --- a/test_regress/t/t_event_copy.out +++ b/test_regress/t/t_event_copy.out @@ -1,10 +1,17 @@ -%Error-UNSUPPORTED: t/t_event_copy.v:100:13: Unsupported: assignment of event data type +%Error-UNSUPPORTED: t/t_event_copy.v:100:13: Assignement to and from event in statically scheduled context. : ... note: In instance 't' + : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. 100 | e4 = e3; | ^ ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest -%Error-UNSUPPORTED: t/t_event_copy.v:101:13: Unsupported: assignment of event data type +%Error-UNSUPPORTED: t/t_event_copy.v:101:13: Assignement to and from event in statically scheduled context. : ... note: In instance 't' + : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. 101 | e3 = e2; | ^ +%Error-UNSUPPORTED: t/t_event_copy.v:128:13: Assignement to event in statically scheduled context. + : ... note: In instance 't' + : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. + 128 | e3 = null; + | ^ %Error: Exiting due to From 7b62a6f53acdb5fcdf4e2ad30dce0c6d708dd49c Mon Sep 17 00:00:00 2001 From: github action Date: Mon, 7 Aug 2023 14:40:51 +0000 Subject: [PATCH 10/27] Apply 'make format' --- src/V3Width.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 804d6cb827..a601dfdf0a 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4806,8 +4806,7 @@ class WidthVisitor final : public VNVisitor { // return; } - if (nodep->dtypep()->isEvent()) - checkEventAssignement(nodep); + if (nodep->dtypep()->isEvent()) checkEventAssignement(nodep); } void visit(AstRelease* nodep) override { From 9c2f4cf56d81c4ca195d208e72771c466456636b Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 7 Aug 2023 18:04:28 +0200 Subject: [PATCH 11/27] Fix use-after-free Signed-off-by: Krzysztof Boronski --- src/V3Width.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index a601dfdf0a..050d46272a 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4803,10 +4803,10 @@ class WidthVisitor final : public VNVisitor { newp->dtypeSetVoid(); nodep->replaceWith(newp->makeStmt()); VL_DO_DANGLING(pushDeletep(nodep), nodep); - // return; + return; } - if (nodep->dtypep()->isEvent()) checkEventAssignement(nodep); + if (nodep->hasDType() && nodep->dtypep()->isEvent()) checkEventAssignement(nodep); } void visit(AstRelease* nodep) override { From a6f642845a7b185fb92f5eeb48c50dbd038f6996 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 7 Aug 2023 18:06:20 +0200 Subject: [PATCH 12/27] Remove fixme - no longer relevant Signed-off-by: Krzysztof Boronski --- src/V3Width.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 050d46272a..c1d73ff52a 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4741,14 +4741,6 @@ class WidthVisitor final : public VNVisitor { iterateCheckAssign(nodep, "Assign RHS", nodep->rhsp(), FINAL, lhsDTypep); // if (debug()) nodep->dumpTree("- AssignOut: "); } - // FIXME: This should still report an error in cases where events would be handled - // by static scheduler (__VtrigSched), ie. evaluated in active loop. - // if (const AstBasicDType* const basicp = nodep->rhsp()->dtypep()->basicp()) { - // if (basicp->isEvent()) { - // // see t_event_copy.v for commentary on the mess involved - // nodep->v3warn(E_UNSUPPORTED, "Unsupported: assignment of event data type"); - // } - //} if (auto* const controlp = nodep->timingControlp()) { if (VN_IS(m_ftaskp, Func)) { controlp->v3error("Timing controls are not legal in functions. Suggest use a task " From ef9f90179185685ff39c03b716cb4d397052ce6e Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 7 Aug 2023 20:55:19 +0200 Subject: [PATCH 13/27] Choose between event implementations depending on event assigneent presence Signed-off-by: Krzysztof Boronski --- include/verilated_types.h | 13 +++++++++++++ src/V3EmitCMain.cpp | 2 ++ src/V3EmitCSyms.cpp | 20 +++++++++++++++++--- src/V3Global.h | 3 +++ src/V3Width.cpp | 5 ++++- 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/include/verilated_types.h b/include/verilated_types.h index 7dc4b4c3ec..e5ebf1fbbc 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -174,13 +174,23 @@ class VlEvent final { ~VlEvent() = default; friend std::string VL_TO_STRING(const VlEvent& e); +#ifdef VL_EVENT_ASSIGN friend class VlEventHandle; +#else + // METHODS + void fire() { m_fired = m_triggered = true; } + bool isFired() const { return m_fired; } + bool isTriggered() const { return m_triggered; } + void clearFired() { m_fired = false; } + void clearTriggered() { m_triggered = false; } +#endif }; inline std::string VL_TO_STRING(const VlEvent& e) { return std::string{"triggered="} + (e.m_triggered ? "true" : "false"); } +#ifdef VL_EVENT_ASSIGN class VlEventHandle final : public std::shared_ptr { public: // Constructor @@ -196,6 +206,9 @@ class VlEventHandle final : public std::shared_ptr { }; inline std::string VL_TO_STRING(const VlEventHandle& e) { return "&{ " + VL_TO_STRING(*e) + " }"; } +#else +using VlEventHandle = VlEvent; +#endif //=================================================================== // Random diff --git a/src/V3EmitCMain.cpp b/src/V3EmitCMain.cpp index 873519d5d9..b8d929951b 100644 --- a/src/V3EmitCMain.cpp +++ b/src/V3EmitCMain.cpp @@ -53,6 +53,8 @@ class EmitCMain final : EmitCBaseVisitorConst { // Not defining main_time/vl_time_stamp, so v3Global.opt.addCFlags("-DVL_TIME_CONTEXT"); // On MSVC++ anyways + if (v3Global.assignsEvents()) v3Global.opt.addCFlags("-DVL_EVENT_ASSIGN"); + // Optional main top name argument, with empty string replacement string topArg; string topName = v3Global.opt.mainTopName(); diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index 05f69166c5..c7b6554c0d 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -464,7 +464,13 @@ void EmitCSyms::emitSymHdr() { puts("uint32_t __Vm_baseCode = 0;" " ///< Used by trace routines when tracing multiple models\n"); } - if (v3Global.hasEvents()) puts("std::vector __Vm_triggeredEvents;\n"); + if (v3Global.hasEvents()) { + if (v3Global.assignsEvents()) { + puts("std::vector __Vm_triggeredEvents;\n"); + } else { + puts("std::vector __Vm_triggeredEvents;\n"); + } + } if (v3Global.hasClasses()) puts("VlDeleter __Vm_deleter;\n"); puts("bool __Vm_didInit = false;\n"); @@ -539,10 +545,18 @@ void EmitCSyms::emitSymHdr() { "'enqueueTriggeredEventForClearing' was not triggered\");\n"); puts("}\n"); puts("#endif\n"); - puts("__Vm_triggeredEvents.push_back(event);\n"); + if (v3Global.assignsEvents()) { + puts("__Vm_triggeredEvents.push_back(event);\n"); + } else { + puts("__Vm_triggeredEvents.push_back(&event);\n"); + } puts("}\n"); puts("void clearTriggeredEvents() {\n"); - puts("for (auto& eventp : __Vm_triggeredEvents) eventp.clearTriggered();\n"); + if (v3Global.assignsEvents()) { + puts("for (auto& event : __Vm_triggeredEvents) event.clearTriggered();\n"); + } else { + puts("for (const auto eventp : __Vm_triggeredEvents) eventp->clearTriggered();\n"); + } puts("__Vm_triggeredEvents.clear();\n"); puts("}\n"); } diff --git a/src/V3Global.h b/src/V3Global.h index a25a3f274e..1353423bb1 100644 --- a/src/V3Global.h +++ b/src/V3Global.h @@ -103,6 +103,7 @@ class V3Global final { std::atomic_int m_debugFileNumber{0}; // Number to append to debug files created bool m_assertDTypesResolved = false; // Tree should have dtypep()'s bool m_assertScoped = false; // Tree is scoped + bool m_assignsEvents = false; // Design uses assignments on SystemVerilog Events bool m_constRemoveXs = false; // Const needs to strip any Xs // Experimenting with always requiring heavy, see issue #2701 bool m_needTraceDumper = false; // Need __Vm_dumperp in symbols @@ -153,6 +154,8 @@ class V3Global final { void needTraceDumper(bool flag) { m_needTraceDumper = flag; } bool dpi() const VL_MT_SAFE { return m_dpi; } void dpi(bool flag) { m_dpi = flag; } + bool assignsEvents() const { return m_assignsEvents; } + void setAssignsEvents() { m_assignsEvents = true; } bool hasEvents() const { return m_hasEvents; } void setHasEvents() { m_hasEvents = true; } bool hasClasses() const { return m_hasClasses; } diff --git a/src/V3Width.cpp b/src/V3Width.cpp index c1d73ff52a..7f891a989b 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4798,7 +4798,10 @@ class WidthVisitor final : public VNVisitor { return; } - if (nodep->hasDType() && nodep->dtypep()->isEvent()) checkEventAssignement(nodep); + if (nodep->hasDType() && nodep->dtypep()->isEvent()) { + checkEventAssignement(nodep); + v3Global.setAssignsEvents(); + } } void visit(AstRelease* nodep) override { From 04e07b89c8904c884f07730782bd19a23008bd02 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Tue, 10 Oct 2023 18:44:40 +0200 Subject: [PATCH 14/27] Fix CCast for event types Signed-off-by: Krzysztof Boronski --- src/V3Cast.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/V3Cast.cpp b/src/V3Cast.cpp index 4fb3b868ed..9b734be76a 100644 --- a/src/V3Cast.cpp +++ b/src/V3Cast.cpp @@ -193,7 +193,7 @@ class CastVisitor final : public VNVisitor { && !VN_IS(backp, ArraySel) && !VN_IS(backp, StructSel) && !VN_IS(backp, RedXor) && (nodep->varp()->basicp() && !nodep->varp()->basicp()->isTriggerVec() && !nodep->varp()->basicp()->isForkSync() - && !nodep->varp()->basicp()->isProcessRef()) + && !nodep->varp()->basicp()->isProcessRef() && !nodep->varp()->basicp()->isEvent()) && backp->width() && castSize(nodep) != castSize(nodep->varp())) { // Cast vars to IData first, else below has upper bits wrongly set // CData x=3; out = (QData)(x<<30); From 39e9969c71f3ca8d9b632f1ba4c28bf910843cba Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 11 Oct 2023 14:37:22 +0200 Subject: [PATCH 15/27] Use -Wno-EVENTEXPR for uvm_todo test Signed-off-by: Krzysztof Boronski --- test_regress/t/t_uvm_todo.pl | 1 + 1 file changed, 1 insertion(+) diff --git a/test_regress/t/t_uvm_todo.pl b/test_regress/t/t_uvm_todo.pl index 9fb3de36f6..850402182f 100755 --- a/test_regress/t/t_uvm_todo.pl +++ b/test_regress/t/t_uvm_todo.pl @@ -14,6 +14,7 @@ v_flags2 => ["--timing", "-Wno-PKGNODECL -Wno-IMPLICITSTATIC -Wno-CONSTRAINTIGN -Wno-MISINDENT", "-Wno-CASEINCOMPLETE -Wno-CASTCONST -Wno-SYMRSVDWORD -Wno-WIDTHEXPAND -Wno-WIDTHTRUNC", + "-Wno-EVENTEXPR", "-Wno-REALCVT", # TODO note mostly related to $realtime - could suppress or fix upstream "-Wno-ZERODLY", # TODO issue #4494, add support ], From f6a0ea74a639da889f05de0d145258741a76563a Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 11 Oct 2023 16:42:50 +0200 Subject: [PATCH 16/27] Create a function for detailed check for hierarchical identifiers Signed-off-by: Krzysztof Boronski --- src/V3LinkDot.cpp | 28 +++++++++++++++---- .../t/t_event_control_eventexpr_bad.v | 21 ++++++++++++-- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 5db83da293..6028986bf3 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2273,6 +2273,25 @@ class LinkDotResolveVisitor final : public VNVisitor { if (nodep && nodep->isParam()) nodep->usedParam(true); } + static bool isTreeHierarchicalIdentExpr(AstNode* nodep) { + while (true) { + if (const AstMemberSel* mselp = VN_CAST(nodep, MemberSel)) { + nodep = mselp->fromp(); + continue; + } + if (const AstSelBit* selbp = VN_CAST(nodep, SelBit)) { + if (VN_IS(selbp->bitp(), Const)) { + nodep = selbp->fromp(); + continue; + } else { + return false; + } + } + break; + } + return VN_IS(nodep, VarRef); + } + // VISITs void visit(AstNetlist* nodep) override { // Recurse..., backward as must do packages before using packages @@ -3383,11 +3402,10 @@ class LinkDotResolveVisitor final : public VNVisitor { } void visit(AstFireEvent* nodep) override { visit(static_cast(nodep)); - const AstNodeExpr* operandp = nodep->operandp(); - while (const AstMemberSel* mselp = VN_CAST(operandp, MemberSel)) operandp = mselp->fromp(); - if (!VN_IS(operandp, VarRef)) { - operandp->v3warn(EVENTEXPR, "Non-identifier expression used to reference an event to " - "be sent. This is not a part of IEEE_1800-2017"); + if (!isTreeHierarchicalIdentExpr(nodep->operandp())) { + nodep->operandp()->v3warn(EVENTEXPR, + "Non-identifier expression used to reference an event to " + "be sent. This is not a part of IEEE_1800-2017"); } } void visit(AstWith* nodep) override { diff --git a/test_regress/t/t_event_control_eventexpr_bad.v b/test_regress/t/t_event_control_eventexpr_bad.v index a2e544ecaf..fe7433f0ba 100644 --- a/test_regress/t/t_event_control_eventexpr_bad.v +++ b/test_regress/t/t_event_control_eventexpr_bad.v @@ -5,11 +5,28 @@ // SPDX-License-Identifier: CC0-1.0 class Foo; + Foo other; + Foo other_array[10]; event evt; + event evt_array[10]; + function automatic event get_event(); return evt; endfunction - task send_event(); - ->get_event(); + + task send_events(); + ->evt; // Should be good + ->get_event(); // Should fail - it's not a hierarchical identifier + ->other.evt; // Should be good + for (int i = 0; i < 10; i++) begin + ->evt_array[i]; // Should fail - it's an expression that's not a + // hierarhical identifier, because of the variable + // used for indexing + end + // Should be good because we are using constant selectors + ->evt_array[2]; + ->other.evt_array[1]; + ->other_array[3].evt; + ->other_array[0].evt_array[7]; endtask endclass From 350fe6da5686628a17f5391c930dcf401ec409e3 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 11 Oct 2023 17:25:22 +0200 Subject: [PATCH 17/27] Update t_event_control_eventexpr_bad expected output Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_eventexpr_bad.out | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test_regress/t/t_event_control_eventexpr_bad.out b/test_regress/t/t_event_control_eventexpr_bad.out index c83d3afd31..23097c8d2b 100644 --- a/test_regress/t/t_event_control_eventexpr_bad.out +++ b/test_regress/t/t_event_control_eventexpr_bad.out @@ -1,6 +1,10 @@ -%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:13:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 - 13 | ->get_event(); +%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:19:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 + 19 | ->get_event(); | ^~~~~~~~~ ... For warning description see https://verilator.org/warn/EVENTEXPR?v=latest ... Use "/* verilator lint_off EVENTEXPR */" and lint_on around source to disable this message. -%Error: Exiting due to +%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:22:18: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 + 22 | ->evt_array[i]; + | ^ +%Error: Verilator internal fault, sorry. Suggest trying --debug --gdbbt +%Error: Command Failed From e4977913385e59c9536451a7be19ef3ff6a7f80f Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 11 Oct 2023 18:08:47 +0200 Subject: [PATCH 18/27] Add a warning (with test) for assigning events in multithreaded context Signed-off-by: Krzysztof Boronski --- src/V3Error.h | 3 ++- src/V3Width.cpp | 5 +++++ test_regress/t/t_event_control_assign_mt.out | 6 ++++++ test_regress/t/t_event_control_assign_mt.pl | 22 ++++++++++++++++++++ test_regress/t/t_event_control_assign_mt.v | 14 +++++++++++++ 5 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 test_regress/t/t_event_control_assign_mt.out create mode 100755 test_regress/t/t_event_control_assign_mt.pl create mode 100644 test_regress/t/t_event_control_assign_mt.v diff --git a/src/V3Error.h b/src/V3Error.h index 28e2804f55..e00c2a7b54 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -99,6 +99,7 @@ class V3ErrorCode final { ENUMVALUE, // Error: enum type needs explicit cast EOFNEWLINE, // End-of-file missing newline EVENTEXPR, // Non-identifier expression used to send an event + EVENTMT, // Dangerous event operation in multithreaded simulation GENCLK, // Generated Clock. Historical, never issued. GENUNNAMED, // Generate unnamed, without label HIERBLOCK, // Ignored hierarchical block setting @@ -195,7 +196,7 @@ class V3ErrorCode final { "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", "CDCRSTLOGIC", "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", "CONTASSREG", "DECLFILENAME", "DEFPARAM", "DEPRECATED", - "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "EVENTEXPR", "GENCLK", + "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "EVENTEXPR", "EVENTMT", "GENCLK", "GENUNNAMED", "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 7f891a989b..ba05abd67b 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4801,6 +4801,11 @@ class WidthVisitor final : public VNVisitor { if (nodep->hasDType() && nodep->dtypep()->isEvent()) { checkEventAssignement(nodep); v3Global.setAssignsEvents(); + if (v3Global.opt.threads() > 1) { + nodep->v3warn(EVENTMT, + "Event assignements might cause issues in multithreaded simulation. " + "Use at your own risk."); + } } } diff --git a/test_regress/t/t_event_control_assign_mt.out b/test_regress/t/t_event_control_assign_mt.out new file mode 100644 index 0000000000..e1c4242505 --- /dev/null +++ b/test_regress/t/t_event_control_assign_mt.out @@ -0,0 +1,6 @@ +%Warning-EVENTMT: t/t_event_control_assign_mt.v:12:10: Event assignements might cause issues in multithreaded simulation. Use at your own risk. + 12 | evt1 = evt2; + | ^ + ... For warning description see https://verilator.org/warn/EVENTMT?v=latest + ... Use "/* verilator lint_off EVENTMT */" and lint_on around source to disable this message. +%Error: Exiting due to diff --git a/test_regress/t/t_event_control_assign_mt.pl b/test_regress/t/t_event_control_assign_mt.pl new file mode 100755 index 0000000000..a8b9fc41fc --- /dev/null +++ b/test_regress/t/t_event_control_assign_mt.pl @@ -0,0 +1,22 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2023 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(vltmt => 1); + +compile( + verilator_flags2 => ["--exe --main --timing"], + make_main => 0, + threads => 2, + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_event_control_assign_mt.v b/test_regress/t/t_event_control_assign_mt.v new file mode 100644 index 0000000000..a8c3ec583a --- /dev/null +++ b/test_regress/t/t_event_control_assign_mt.v @@ -0,0 +1,14 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + + +class Foo; + event evt1; + task automatic assign_new(); + event evt2; + evt1 = evt2; + endtask +endclass From 3004813f8448d497ca1832e3186b98289e7d60c7 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 11 Oct 2023 18:58:12 +0200 Subject: [PATCH 19/27] Disable multithreading for some event assign tests Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_assign.pl | 2 ++ test_regress/t/t_event_control_pass.pl | 1 + test_regress/t/t_event_copy.pl | 1 + 3 files changed, 4 insertions(+) diff --git a/test_regress/t/t_event_control_assign.pl b/test_regress/t/t_event_control_assign.pl index 00a80e5f2a..06ac175ad2 100755 --- a/test_regress/t/t_event_control_assign.pl +++ b/test_regress/t/t_event_control_assign.pl @@ -13,6 +13,8 @@ compile( verilator_flags2 => ["--exe --main --timing"], make_main => 0, + # Multithreading would cause a warning on event assignments + threads => 1, ); execute( diff --git a/test_regress/t/t_event_control_pass.pl b/test_regress/t/t_event_control_pass.pl index 9d09ae0b12..1c0f611960 100755 --- a/test_regress/t/t_event_control_pass.pl +++ b/test_regress/t/t_event_control_pass.pl @@ -13,6 +13,7 @@ compile( verilator_flags2 => ["--exe --main --timing -Wno-EVENTEXPR"], make_main => 0, + threads => 1, ); execute( diff --git a/test_regress/t/t_event_copy.pl b/test_regress/t/t_event_copy.pl index be66c40e69..bce8f74ca8 100755 --- a/test_regress/t/t_event_copy.pl +++ b/test_regress/t/t_event_copy.pl @@ -13,6 +13,7 @@ compile( fails => $Self->{vlt_all}, expect_filename => $Self->{golden_filename}, + threads => 1, ); execute( From 6d76ac0dfad07cb532d320db11ae642212c1f7eb Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Tue, 17 Oct 2023 19:44:17 +0200 Subject: [PATCH 20/27] Remove junk from t_event_control_pass Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_pass.v | 3 --- 1 file changed, 3 deletions(-) diff --git a/test_regress/t/t_event_control_pass.v b/test_regress/t/t_event_control_pass.v index 17aebd6f96..5304a829b4 100644 --- a/test_regress/t/t_event_control_pass.v +++ b/test_regress/t/t_event_control_pass.v @@ -48,7 +48,4 @@ module t(); $write("*-* All Finished *-*\n"); $finish; end - - initial begin - end endmodule From ab41d31aa9ee800937022d34dc5b35b94c89ac0a Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Tue, 17 Oct 2023 19:53:15 +0200 Subject: [PATCH 21/27] Improve error messages Signed-off-by: Krzysztof Boronski --- src/V3LinkDot.cpp | 2 +- src/V3Width.cpp | 5 +++-- test_regress/t/t_event_copy.out | 9 ++++++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 6028986bf3..60baa163f2 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -3405,7 +3405,7 @@ class LinkDotResolveVisitor final : public VNVisitor { if (!isTreeHierarchicalIdentExpr(nodep->operandp())) { nodep->operandp()->v3warn(EVENTEXPR, "Non-identifier expression used to reference an event to " - "be sent. This is not a part of IEEE_1800-2017"); + "be sent. This is not a part of IEEE 1800-2017."); } } void visit(AstWith* nodep) override { diff --git a/src/V3Width.cpp b/src/V3Width.cpp index ba05abd67b..4a84dd1eda 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4319,8 +4319,9 @@ class WidthVisitor final : public VNVisitor { << " event in statically scheduled context.\n" << asgnp->warnMore() << "Static event " - "scheduling won't be able to handle this. " - "Your best bet is to move the event into a " + "scheduling won't be able to handle this.\n" + << asgnp->warnMore() << + "... Suggest move the event into a " "completely dynamic context, eg. a class, and " "reference it only from such context."); } diff --git a/test_regress/t/t_event_copy.out b/test_regress/t/t_event_copy.out index f802a38b54..8784c2883d 100644 --- a/test_regress/t/t_event_copy.out +++ b/test_regress/t/t_event_copy.out @@ -1,17 +1,20 @@ %Error-UNSUPPORTED: t/t_event_copy.v:100:13: Assignement to and from event in statically scheduled context. : ... note: In instance 't' - : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. + : Static event scheduling won't be able to handle this. + : ... Suggest move the event into a completely dynamic context, eg. a class, and reference it only from such context. 100 | e4 = e3; | ^ ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest %Error-UNSUPPORTED: t/t_event_copy.v:101:13: Assignement to and from event in statically scheduled context. : ... note: In instance 't' - : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. + : Static event scheduling won't be able to handle this. + : ... Suggest move the event into a completely dynamic context, eg. a class, and reference it only from such context. 101 | e3 = e2; | ^ %Error-UNSUPPORTED: t/t_event_copy.v:128:13: Assignement to event in statically scheduled context. : ... note: In instance 't' - : Static event scheduling won't be able to handle this. Your best bet is to move the event into a completely dynamic context, eg. a class, and reference it only from such context. + : Static event scheduling won't be able to handle this. + : ... Suggest move the event into a completely dynamic context, eg. a class, and reference it only from such context. 128 | e3 = null; | ^ %Error: Exiting due to From e7fe3a39cefb4539d1930d0c708c5f7511bc1caa Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Tue, 17 Oct 2023 19:55:30 +0200 Subject: [PATCH 22/27] remove relevant uvm todo Signed-off-by: Krzysztof Boronski --- test_regress/t/t_uvm_pkg_todo.vh | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test_regress/t/t_uvm_pkg_todo.vh b/test_regress/t/t_uvm_pkg_todo.vh index f3f21fa0df..fe5ff5d430 100644 --- a/test_regress/t/t_uvm_pkg_todo.vh +++ b/test_regress/t/t_uvm_pkg_todo.vh @@ -7474,9 +7474,7 @@ virtual class uvm_event_base extends uvm_object; event e; if (wakeup) ->m_event; -//TODO issue #4468 - Fix UVM assignment of event data types -//TODO %Error-UNSUPPORTED: t/t_uvm_pkg_todo.vh:7477:25: Unsupported: assignment of event data type -//TODO m_event = e; + m_event = e; num_waiters = 0; on = 0; trigger_time = 0; @@ -7497,9 +7495,7 @@ virtual class uvm_event_base extends uvm_object; uvm_event_base e; super.do_copy(rhs); if(!$cast(e, rhs) || (e==null)) return; -//TODO issue #4468 - Fix UVM assignment of event data types -//TODO %Error-UNSUPPORTED: t/t_uvm_pkg_todo.vh:7498:25: Unsupported: assignment of event data type -//TODO m_event = e.m_event; + m_event = e.m_event; num_waiters = e.num_waiters; on = e.on; trigger_time = e.trigger_time; From fac02e240e7c550500d084d9128913d4e42636c7 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 18 Oct 2023 18:41:59 +0200 Subject: [PATCH 23/27] Update t_event_control_eventexpr_bad.out Signed-off-by: Krzysztof Boronski --- test_regress/t/t_event_control_eventexpr_bad.out | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_regress/t/t_event_control_eventexpr_bad.out b/test_regress/t/t_event_control_eventexpr_bad.out index 23097c8d2b..7736f7a918 100644 --- a/test_regress/t/t_event_control_eventexpr_bad.out +++ b/test_regress/t/t_event_control_eventexpr_bad.out @@ -1,9 +1,9 @@ -%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:19:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 +%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:19:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE 1800-2017. 19 | ->get_event(); | ^~~~~~~~~ ... For warning description see https://verilator.org/warn/EVENTEXPR?v=latest ... Use "/* verilator lint_off EVENTEXPR */" and lint_on around source to disable this message. -%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:22:18: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE_1800-2017 +%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:22:18: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE 1800-2017. 22 | ->evt_array[i]; | ^ %Error: Verilator internal fault, sorry. Suggest trying --debug --gdbbt From 4518d7e2796666e8476acb0b003cc96114e80b3d Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Wed, 18 Oct 2023 21:33:10 +0200 Subject: [PATCH 24/27] Allow Assignable and non-assignable events to co-exist, format Signed-off-by: Krzysztof Boronski --- include/verilated_fst_c.cpp | 2 +- include/verilated_fst_c.h | 2 +- include/verilated_trace.h | 6 +-- include/verilated_trace_imp.h | 4 +- include/verilated_types.h | 70 +++++++++++++++++++++-------------- include/verilated_vcd_c.cpp | 4 +- include/verilated_vcd_c.h | 2 +- src/V3Ast.h | 2 +- src/V3AstNodes.cpp | 2 +- src/V3EmitCImp.cpp | 3 ++ src/V3EmitCSyms.cpp | 10 +++-- src/V3Width.cpp | 4 +- 12 files changed, 67 insertions(+), 44 deletions(-) diff --git a/include/verilated_fst_c.cpp b/include/verilated_fst_c.cpp index b3dc26c8dc..5924bc5b87 100644 --- a/include/verilated_fst_c.cpp +++ b/include/verilated_fst_c.cpp @@ -284,7 +284,7 @@ void VerilatedFst::configure(const VerilatedTraceConfig& config) { // so always inline them. VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitEvent(uint32_t code, VlEventHandle newval) { +void VerilatedFstBuffer::emitEvent(uint32_t code, const VlEventBase* newval) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); fstWriterEmitValueChange(m_fst, m_symbolp[code], "1"); } diff --git a/include/verilated_fst_c.h b/include/verilated_fst_c.h index c3a6eacbdd..07e1102aa2 100644 --- a/include/verilated_fst_c.h +++ b/include/verilated_fst_c.h @@ -163,7 +163,7 @@ class VerilatedFstBuffer VL_NOT_FINAL { // Implementations of duck-typed methods for VerilatedTraceBuffer. These are // called from only one place (the full* methods), so always inline them. - VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEventHandle newval); + VL_ATTR_ALWINLINE void emitEvent(uint32_t code, const VlEventBase* newval); VL_ATTR_ALWINLINE void emitBit(uint32_t code, CData newval); VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int bits); VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int bits); diff --git a/include/verilated_trace.h b/include/verilated_trace.h index 4a969ded26..4eee175c82 100644 --- a/include/verilated_trace.h +++ b/include/verilated_trace.h @@ -423,7 +423,7 @@ class VerilatedTraceBuffer VL_NOT_FINAL : public T_Buffer { void fullQData(uint32_t* oldp, QData newval, int bits); void fullWData(uint32_t* oldp, const WData* newvalp, int bits); void fullDouble(uint32_t* oldp, double newval); - void fullEvent(uint32_t* oldp, VlEventHandle newval); + void fullEvent(uint32_t* oldp, const VlEventBase* newval); // In non-offload mode, these are called directly by the trace callbacks, // and are called chg*. In offload mode, they are called by the worker @@ -460,7 +460,7 @@ class VerilatedTraceBuffer VL_NOT_FINAL : public T_Buffer { } } } - VL_ATTR_ALWINLINE void chgEvent(uint32_t* oldp, VlEventHandle newval) { + VL_ATTR_ALWINLINE void chgEvent(uint32_t* oldp, const VlEventBase* newval) { fullEvent(oldp, newval); } VL_ATTR_ALWINLINE void chgDouble(uint32_t* oldp, double newval) { @@ -541,7 +541,7 @@ class VerilatedTraceOffloadBuffer final : public VerilatedTraceBuffer m_offloadBufferWritep += 4; VL_DEBUG_IF(assert(m_offloadBufferWritep <= m_offloadBufferEndp);); } - void chgEvent(uint32_t code, VlEventHandle newval) { + void chgEvent(uint32_t code, const VlEventBase* newval) { m_offloadBufferWritep[0] = VerilatedTraceOffloadCommand::CHG_EVENT; m_offloadBufferWritep[1] = code; m_offloadBufferWritep += 2; diff --git a/include/verilated_trace_imp.h b/include/verilated_trace_imp.h index 5a16a1176e..a66792c656 100644 --- a/include/verilated_trace_imp.h +++ b/include/verilated_trace_imp.h @@ -185,7 +185,7 @@ void VerilatedTrace::offloadWorkerThreadMain() { continue; case VerilatedTraceOffloadCommand::CHG_EVENT: VL_TRACE_OFFLOAD_DEBUG("Command CHG_EVENT " << top); - traceBufp->chgEvent(oldp, *reinterpret_cast(readp)); + traceBufp->chgEvent(oldp, reinterpret_cast(readp)); continue; //=== @@ -839,7 +839,7 @@ void VerilatedTraceBuffer::fullBit(uint32_t* oldp, CData newval) { } template <> -void VerilatedTraceBuffer::fullEvent(uint32_t* oldp, VlEventHandle newval) { +void VerilatedTraceBuffer::fullEvent(uint32_t* oldp, const VlEventBase* newval) { const uint32_t code = oldp - m_sigs_oldvalp; *oldp = 1; // Do we really store an "event" ? emitEvent(code, newval); diff --git a/include/verilated_types.h b/include/verilated_types.h index e5ebf1fbbc..66475a88de 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -163,7 +163,18 @@ class VlTriggerVec final { //=================================================================== // SystemVerilog event type -class VlEvent final { +class VlEventBase VL_NOT_FINAL { +public: + virtual ~VlEventBase() = default; + + virtual void fire() = 0; + virtual bool isFired() const = 0; + virtual bool isTriggered() const = 0; + virtual void clearFired() = 0; + virtual void clearTriggered() = 0; +}; + +class VlEvent final : public VlEventBase { // MEMBERS bool m_fired = false; // Fired on this scheduling iteration bool m_triggered = false; // Triggered state of event persisting until next time step @@ -171,44 +182,49 @@ class VlEvent final { public: // CONSTRUCTOR VlEvent() = default; - ~VlEvent() = default; + ~VlEvent() override = default; friend std::string VL_TO_STRING(const VlEvent& e); -#ifdef VL_EVENT_ASSIGN - friend class VlEventHandle; -#else + friend class VlAssignableEvent; // METHODS - void fire() { m_fired = m_triggered = true; } - bool isFired() const { return m_fired; } - bool isTriggered() const { return m_triggered; } - void clearFired() { m_fired = false; } - void clearTriggered() { m_triggered = false; } -#endif + void fire() override { m_fired = m_triggered = true; } + bool isFired() const override { return m_fired; } + bool isTriggered() const override { return m_triggered; } + void clearFired() override { m_fired = false; } + void clearTriggered() override { m_triggered = false; } }; -inline std::string VL_TO_STRING(const VlEvent& e) { - return std::string{"triggered="} + (e.m_triggered ? "true" : "false"); -} - -#ifdef VL_EVENT_ASSIGN -class VlEventHandle final : public std::shared_ptr { +class VlAssignableEvent final : public std::shared_ptr, public VlEventBase { public: // Constructor - VlEventHandle() + VlAssignableEvent() : std::shared_ptr(new VlEvent) {} + ~VlAssignableEvent() override = default; // METHODS - void fire() { (*this)->m_fired = (*this)->m_triggered = true; } - bool isFired() const { return (*this)->m_fired; } - bool isTriggered() const { return (*this)->m_triggered; } - void clearFired() { (*this)->m_fired = false; } - void clearTriggered() { (*this)->m_triggered = false; } + void fire() override { (*this)->m_fired = (*this)->m_triggered = true; } + bool isFired() const override { return (*this)->m_fired; } + bool isTriggered() const override { return (*this)->m_triggered; } + void clearFired() override { (*this)->m_fired = false; } + void clearTriggered() override { (*this)->m_triggered = false; } }; -inline std::string VL_TO_STRING(const VlEventHandle& e) { return "&{ " + VL_TO_STRING(*e) + " }"; } -#else -using VlEventHandle = VlEvent; -#endif +inline std::string VL_TO_STRING(const VlEventBase& e); + +inline std::string VL_TO_STRING(const VlEvent& e) { + return std::string{"triggered="} + (e.isTriggered() ? "true" : "false"); +} + +inline std::string VL_TO_STRING(const VlAssignableEvent& e) { + return "&{ " + VL_TO_STRING(*e) + " }"; +} + +inline std::string VL_TO_STRING(const VlEventBase& e) { + if (const VlAssignableEvent& assignable = dynamic_cast(e)) { + return VL_TO_STRING(assignable); + } + return std::string{"triggered="} + (e.isTriggered() ? "true" : "false"); +} //=================================================================== // Random diff --git a/include/verilated_vcd_c.cpp b/include/verilated_vcd_c.cpp index b57babeaa9..9ca46e062a 100644 --- a/include/verilated_vcd_c.cpp +++ b/include/verilated_vcd_c.cpp @@ -669,8 +669,8 @@ void VerilatedVcdBuffer::finishLine(uint32_t code, char* writep) { // so always inline them. VL_ATTR_ALWINLINE -void VerilatedVcdBuffer::emitEvent(uint32_t code, VlEventHandle newval) { - const bool triggered = newval.isTriggered(); +void VerilatedVcdBuffer::emitEvent(uint32_t code, const VlEventBase* newval) { + const bool triggered = newval->isTriggered(); // TODO : It seems that untriggered events are not filtered // should be tested before this last step if (triggered) { diff --git a/include/verilated_vcd_c.h b/include/verilated_vcd_c.h index d806f6214a..a07508f62e 100644 --- a/include/verilated_vcd_c.h +++ b/include/verilated_vcd_c.h @@ -207,7 +207,7 @@ class VerilatedVcdBuffer VL_NOT_FINAL { // Implementation of VerilatedTraceBuffer interface // Implementations of duck-typed methods for VerilatedTraceBuffer. These are // called from only one place (the full* methods), so always inline them. - VL_ATTR_ALWINLINE void emitEvent(uint32_t code, VlEventHandle newval); + VL_ATTR_ALWINLINE void emitEvent(uint32_t code, const VlEventBase* newval); VL_ATTR_ALWINLINE void emitBit(uint32_t code, CData newval); VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int bits); VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int bits); diff --git a/src/V3Ast.h b/src/V3Ast.h index 71a9399979..23ec2bc283 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -324,7 +324,7 @@ class VEdgeType final { ET_BOTHEDGE, // POSEDGE | NEGEDGE (i.e.: 'edge' in Verilog) ET_POSEDGE, ET_NEGEDGE, - ET_EVENT, // VlEvent::isFired + ET_EVENT, // VlEventBase::isFired // Involving an expression ET_TRUE, // diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 7f129df82c..371e63ba0b 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -858,7 +858,7 @@ AstNodeDType::CTypeRecursed AstNodeDType::cTypeRecurse(bool compound) const { } else if (bdtypep->isProcessRef()) { info.m_type = "VlProcessRef"; } else if (bdtypep->isEvent()) { - info.m_type = "VlEventHandle"; + info.m_type = v3Global.assignsEvents() ? "VlAssignableEvent" : "VlEvent"; } else if (dtypep->widthMin() <= 8) { // Handle unpacked arrays; not bdtypep->width info.m_type = "CData" + bitvec; } else if (dtypep->widthMin() <= 16) { diff --git a/src/V3EmitCImp.cpp b/src/V3EmitCImp.cpp index f84e9442c5..d39667a45d 100644 --- a/src/V3EmitCImp.cpp +++ b/src/V3EmitCImp.cpp @@ -812,6 +812,9 @@ class EmitCTrace final : EmitCFunc { void emitTraceValue(AstTraceInc* nodep, int arrayindex) { if (AstVarRef* const varrefp = VN_CAST(nodep->valuep(), VarRef)) { AstVar* const varp = varrefp->varp(); + if (varp->isEvent()) { + puts("&"); + } puts("("); if (emitTraceIsScBigUint(nodep)) { puts("(uint32_t*)"); diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index c7b6554c0d..8254a39999 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -466,9 +466,9 @@ void EmitCSyms::emitSymHdr() { } if (v3Global.hasEvents()) { if (v3Global.assignsEvents()) { - puts("std::vector __Vm_triggeredEvents;\n"); + puts("std::vector __Vm_triggeredEvents;\n"); } else { - puts("std::vector __Vm_triggeredEvents;\n"); + puts("std::vector __Vm_triggeredEvents;\n"); } } if (v3Global.hasClasses()) puts("VlDeleter __Vm_deleter;\n"); @@ -538,7 +538,11 @@ void EmitCSyms::emitSymHdr() { puts("const char* name() { return TOP.name(); }\n"); if (v3Global.hasEvents()) { - puts("void enqueueTriggeredEventForClearing(VlEventHandle& event) {\n"); + if (v3Global.assignsEvents()) { + puts("void enqueueTriggeredEventForClearing(VlAssignableEvent& event) {\n"); + } else { + puts("void enqueueTriggeredEventForClearing(VlEvent& event) {\n"); + } puts("#ifdef VL_DEBUG\n"); puts("if (VL_UNLIKELY(!event.isTriggered())) {\n"); puts("VL_FATAL_MT(__FILE__, __LINE__, __FILE__, \"event passed to " diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 4a84dd1eda..ef3969d5a7 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4320,8 +4320,8 @@ class WidthVisitor final : public VNVisitor { << asgnp->warnMore() << "Static event " "scheduling won't be able to handle this.\n" - << asgnp->warnMore() << - "... Suggest move the event into a " + << asgnp->warnMore() + << "... Suggest move the event into a " "completely dynamic context, eg. a class, and " "reference it only from such context."); } From e4df7c22ad4dec29a6ca5587ddb76fc177f6f5ea Mon Sep 17 00:00:00 2001 From: github action Date: Wed, 18 Oct 2023 19:34:34 +0000 Subject: [PATCH 25/27] Apply 'make format' --- src/V3EmitCImp.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/V3EmitCImp.cpp b/src/V3EmitCImp.cpp index d39667a45d..b461bab4d4 100644 --- a/src/V3EmitCImp.cpp +++ b/src/V3EmitCImp.cpp @@ -812,9 +812,7 @@ class EmitCTrace final : EmitCFunc { void emitTraceValue(AstTraceInc* nodep, int arrayindex) { if (AstVarRef* const varrefp = VN_CAST(nodep->valuep(), VarRef)) { AstVar* const varp = varrefp->varp(); - if (varp->isEvent()) { - puts("&"); - } + if (varp->isEvent()) { puts("&"); } puts("("); if (emitTraceIsScBigUint(nodep)) { puts("(uint32_t*)"); From b66a0550d6da9d54887e716fe048e2e4f08af470 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Thu, 19 Oct 2023 14:16:46 +0200 Subject: [PATCH 26/27] Remove optional VL_EVENT_ASSIGN flag for compilation Signed-off-by: Krzysztof Boronski --- src/V3EmitCMain.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/V3EmitCMain.cpp b/src/V3EmitCMain.cpp index b8d929951b..873519d5d9 100644 --- a/src/V3EmitCMain.cpp +++ b/src/V3EmitCMain.cpp @@ -53,8 +53,6 @@ class EmitCMain final : EmitCBaseVisitorConst { // Not defining main_time/vl_time_stamp, so v3Global.opt.addCFlags("-DVL_TIME_CONTEXT"); // On MSVC++ anyways - if (v3Global.assignsEvents()) v3Global.opt.addCFlags("-DVL_EVENT_ASSIGN"); - // Optional main top name argument, with empty string replacement string topArg; string topName = v3Global.opt.mainTopName(); From e7abbeb4c54f49ae9a7283377d6e6184b9026c74 Mon Sep 17 00:00:00 2001 From: Krzysztof Boronski Date: Mon, 23 Oct 2023 16:02:55 +0200 Subject: [PATCH 27/27] Remove EVENTEXPR and EVENTMT warnings Signed-off-by: Krzysztof Boronski --- src/V3Error.h | 4 +-- src/V3LinkDot.cpp | 27 ---------------- src/V3Width.cpp | 5 --- test_regress/t/t_event_control_assign_mt.out | 6 ---- test_regress/t/t_event_control_assign_mt.pl | 22 ------------- test_regress/t/t_event_control_assign_mt.v | 14 -------- .../t/t_event_control_eventexpr_bad.out | 10 ------ .../t/t_event_control_eventexpr_bad.pl | 20 ------------ .../t/t_event_control_eventexpr_bad.v | 32 ------------------- test_regress/t/t_event_control_pass.pl | 2 +- test_regress/t/t_uvm_todo.pl | 1 - 11 files changed, 2 insertions(+), 141 deletions(-) delete mode 100644 test_regress/t/t_event_control_assign_mt.out delete mode 100755 test_regress/t/t_event_control_assign_mt.pl delete mode 100644 test_regress/t/t_event_control_assign_mt.v delete mode 100644 test_regress/t/t_event_control_eventexpr_bad.out delete mode 100755 test_regress/t/t_event_control_eventexpr_bad.pl delete mode 100644 test_regress/t/t_event_control_eventexpr_bad.v diff --git a/src/V3Error.h b/src/V3Error.h index eafecc151d..acebf7e6bc 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -98,8 +98,6 @@ class V3ErrorCode final { ENDLABEL, // End lable name mismatch ENUMVALUE, // Error: enum type needs explicit cast EOFNEWLINE, // End-of-file missing newline - EVENTEXPR, // Non-identifier expression used to send an event - EVENTMT, // Dangerous event operation in multithreaded simulation GENCLK, // Generated Clock. Historical, never issued. GENUNNAMED, // Generate unnamed, without label HIERBLOCK, // Ignored hierarchical block setting @@ -197,7 +195,7 @@ class V3ErrorCode final { "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", "CDCRSTLOGIC", "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", "CONTASSREG", "DECLFILENAME", "DEFPARAM", "DEPRECATED", - "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "EVENTEXPR", "EVENTMT", "GENCLK", + "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "GENUNNAMED", "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index d9ae6d5691..473545166e 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2269,25 +2269,6 @@ class LinkDotResolveVisitor final : public VNVisitor { if (nodep && nodep->isParam()) nodep->usedParam(true); } - static bool isTreeHierarchicalIdentExpr(AstNode* nodep) { - while (true) { - if (const AstMemberSel* mselp = VN_CAST(nodep, MemberSel)) { - nodep = mselp->fromp(); - continue; - } - if (const AstSelBit* selbp = VN_CAST(nodep, SelBit)) { - if (VN_IS(selbp->bitp(), Const)) { - nodep = selbp->fromp(); - continue; - } else { - return false; - } - } - break; - } - return VN_IS(nodep, VarRef); - } - // VISITs void visit(AstNetlist* nodep) override { // Recurse..., backward as must do packages before using packages @@ -3407,14 +3388,6 @@ class LinkDotResolveVisitor final : public VNVisitor { } m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); } - void visit(AstFireEvent* nodep) override { - visit(static_cast(nodep)); - if (!isTreeHierarchicalIdentExpr(nodep->operandp())) { - nodep->operandp()->v3warn(EVENTEXPR, - "Non-identifier expression used to reference an event to " - "be sent. This is not a part of IEEE 1800-2017."); - } - } void visit(AstWith* nodep) override { UINFO(5, " " << nodep << endl); checkNoDot(nodep); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 344a6eb2b6..808cd0aecb 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -4808,11 +4808,6 @@ class WidthVisitor final : public VNVisitor { if (nodep->hasDType() && nodep->dtypep()->isEvent()) { checkEventAssignement(nodep); v3Global.setAssignsEvents(); - if (v3Global.opt.threads() > 1) { - nodep->v3warn(EVENTMT, - "Event assignements might cause issues in multithreaded simulation. " - "Use at your own risk."); - } } } diff --git a/test_regress/t/t_event_control_assign_mt.out b/test_regress/t/t_event_control_assign_mt.out deleted file mode 100644 index e1c4242505..0000000000 --- a/test_regress/t/t_event_control_assign_mt.out +++ /dev/null @@ -1,6 +0,0 @@ -%Warning-EVENTMT: t/t_event_control_assign_mt.v:12:10: Event assignements might cause issues in multithreaded simulation. Use at your own risk. - 12 | evt1 = evt2; - | ^ - ... For warning description see https://verilator.org/warn/EVENTMT?v=latest - ... Use "/* verilator lint_off EVENTMT */" and lint_on around source to disable this message. -%Error: Exiting due to diff --git a/test_regress/t/t_event_control_assign_mt.pl b/test_regress/t/t_event_control_assign_mt.pl deleted file mode 100755 index a8b9fc41fc..0000000000 --- a/test_regress/t/t_event_control_assign_mt.pl +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env perl -if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } -# DESCRIPTION: Verilator: Verilog Test driver/expect definition -# -# Copyright 2023 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(vltmt => 1); - -compile( - verilator_flags2 => ["--exe --main --timing"], - make_main => 0, - threads => 2, - fails => 1, - expect_filename => $Self->{golden_filename}, - ); - -ok(1); -1; diff --git a/test_regress/t/t_event_control_assign_mt.v b/test_regress/t/t_event_control_assign_mt.v deleted file mode 100644 index a8c3ec583a..0000000000 --- a/test_regress/t/t_event_control_assign_mt.v +++ /dev/null @@ -1,14 +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 Antmicro Ltd. -// SPDX-License-Identifier: CC0-1.0 - - -class Foo; - event evt1; - task automatic assign_new(); - event evt2; - evt1 = evt2; - endtask -endclass diff --git a/test_regress/t/t_event_control_eventexpr_bad.out b/test_regress/t/t_event_control_eventexpr_bad.out deleted file mode 100644 index 7736f7a918..0000000000 --- a/test_regress/t/t_event_control_eventexpr_bad.out +++ /dev/null @@ -1,10 +0,0 @@ -%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:19:7: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE 1800-2017. - 19 | ->get_event(); - | ^~~~~~~~~ - ... For warning description see https://verilator.org/warn/EVENTEXPR?v=latest - ... Use "/* verilator lint_off EVENTEXPR */" and lint_on around source to disable this message. -%Warning-EVENTEXPR: t/t_event_control_eventexpr_bad.v:22:18: Non-identifier expression used to reference an event to be sent. This is not a part of IEEE 1800-2017. - 22 | ->evt_array[i]; - | ^ -%Error: Verilator internal fault, sorry. Suggest trying --debug --gdbbt -%Error: Command Failed diff --git a/test_regress/t/t_event_control_eventexpr_bad.pl b/test_regress/t/t_event_control_eventexpr_bad.pl deleted file mode 100755 index 54bfd3960a..0000000000 --- a/test_regress/t/t_event_control_eventexpr_bad.pl +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env perl -if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } -# DESCRIPTION: Verilator: Verilog Test driver/expect definition -# -# Copyright 2023 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( - verilator_flags2 => ["--timing"], - fails => 1, - expect_filename => $Self->{golden_filename}, - ); - -ok(1); -1; diff --git a/test_regress/t/t_event_control_eventexpr_bad.v b/test_regress/t/t_event_control_eventexpr_bad.v deleted file mode 100644 index fe7433f0ba..0000000000 --- a/test_regress/t/t_event_control_eventexpr_bad.v +++ /dev/null @@ -1,32 +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 Antmicro Ltd. -// SPDX-License-Identifier: CC0-1.0 - -class Foo; - Foo other; - Foo other_array[10]; - event evt; - event evt_array[10]; - - function automatic event get_event(); - return evt; - endfunction - - task send_events(); - ->evt; // Should be good - ->get_event(); // Should fail - it's not a hierarchical identifier - ->other.evt; // Should be good - for (int i = 0; i < 10; i++) begin - ->evt_array[i]; // Should fail - it's an expression that's not a - // hierarhical identifier, because of the variable - // used for indexing - end - // Should be good because we are using constant selectors - ->evt_array[2]; - ->other.evt_array[1]; - ->other_array[3].evt; - ->other_array[0].evt_array[7]; - endtask -endclass diff --git a/test_regress/t/t_event_control_pass.pl b/test_regress/t/t_event_control_pass.pl index 1c0f611960..2f83297914 100755 --- a/test_regress/t/t_event_control_pass.pl +++ b/test_regress/t/t_event_control_pass.pl @@ -11,7 +11,7 @@ scenarios(simulator => 1); compile( - verilator_flags2 => ["--exe --main --timing -Wno-EVENTEXPR"], + verilator_flags2 => ["--exe --main --timing"], make_main => 0, threads => 1, ); diff --git a/test_regress/t/t_uvm_todo.pl b/test_regress/t/t_uvm_todo.pl index 850402182f..9fb3de36f6 100755 --- a/test_regress/t/t_uvm_todo.pl +++ b/test_regress/t/t_uvm_todo.pl @@ -14,7 +14,6 @@ v_flags2 => ["--timing", "-Wno-PKGNODECL -Wno-IMPLICITSTATIC -Wno-CONSTRAINTIGN -Wno-MISINDENT", "-Wno-CASEINCOMPLETE -Wno-CASTCONST -Wno-SYMRSVDWORD -Wno-WIDTHEXPAND -Wno-WIDTHTRUNC", - "-Wno-EVENTEXPR", "-Wno-REALCVT", # TODO note mostly related to $realtime - could suppress or fix upstream "-Wno-ZERODLY", # TODO issue #4494, add support ],