Deprecate sensitivity list on public_flat_rw attributes (#6443)

These are no longer required for correct scheduling. They are still
accepted for backward compatibility, but have no effect on simulation
and are dropped in the front-end. Also removed the then redundant
AstAlwaysPublic class.

Fixes #6442

docs/guide/connecting.rst

@@ -423,9 +423,9 @@ be deferred for later.  These delayed values can be flushed to the model with
 VPI Example
 -----------
 
-In the below example, we have readme marked read-only, and writeme which if
+In the below example, we have readme marked read-only, and writeme marked
-written from outside the model will have the same semantics as if it
+read-write which if written from outside the model will have the same semantics
-changed on the specified clock edge.
+as if it was a top level input.
 
 .. code-block:: bash
 
@@ -434,7 +434,7 @@ changed on the specified clock edge.
           parameter WIDTH /*verilator public_flat_rd*/ = 32
        ) (input clk);
           reg [WIDTH-1:0] readme   /*verilator public_flat_rd*/;
-          reg [WIDTH-1:0] writeme  /*verilator public_flat_rw @(posedge clk) */;
+          reg [WIDTH-1:0] writeme  /*verilator public_flat_rw*/;
           initial $finish;
        endmodule
      EOF

docs/guide/exe_verilator.rst

@@ -1348,13 +1348,12 @@ Summary:
 .. option:: --public-flat-rw
 
    Declares all variables, ports, and wires public as if they had
-   :code:`/*verilator public_flat_rw @ (<variable's_source_process_edge>)*/`
+   :code:`/*verilator public_flat_rw*/` metacomments.  This will make them VPI
-   metacomments.  This will make them VPI accessible by their flat name,
+   accessible by their flat name, but not turn off module inlining.  This is
-   but not turn off module inlining.  This is particularly useful in
+   particularly useful in combination with :vlopt:`--vpi`. This may also in
-   combination with :vlopt:`--vpi`. This may also in some rare cases result
+   some rare cases result in mis-simulation of generated clocks.  Instead of
-   in mis-simulation of generated clocks.  Instead of this global option,
+   this global option, marking only those signals that need public_flat_rw is
-   marking only those signals that need public_flat_rw is typically
+   typically significantly better performing.
-   significantly better performing.
 
 .. option:: --public-ignore
 
@@ -2338,7 +2337,7 @@ The grammar of control commands is as follows:
 
 .. option:: public_flat_rd [-module "<modulename>"] [-task/-function "<taskname>"] [-var "<signame>"]
 
-.. option:: public_flat_rw [-module "<modulename>"] [-task/-function "<taskname>"] [-var "<signame>" "@(edge)"]
+.. option:: public_flat_rw [-module "<modulename>"] [-task/-function "<taskname>"] [-var "<signame>"] ["@(edge)"]
 
    Sets the variable to be public.  Same as
    :option:`/*verilator&32;public*/` or

docs/guide/extensions.rst

@@ -505,13 +505,14 @@ or "`ifdef`"'s may break other tools.
 
    Same as :option:`public_flat_rd` control file option.
 
-.. option:: /*verilator&32;public_flat_rw @(<edge_list>)*/ (on variable)
+.. option:: /*verilator&32;public_flat_rw [@(<edge_list>)]*/ (on variable)
 
-   Used after an input, output, register, or wire declaration to indicate
+   Used after an input, output, register, or wire declaration to indicate the
-   the signal should be declared public_flat_rd (see above), and writable,
+   signal should be declared public_flat_rd (see above), and writable. Use of
-   where writes should be considered to have the timing specified by the
+   this is implied when using the :vlopt:`--public-flat-rw` option. The edge
-   given sensitivity edge list. Use of this is implied when using the
+   list is optional and has no effect (ignored). Prior to Verilator v5.024 the
-   :vlopt:`--public-flat-rw` option.
+   edge list speciefied the timing when writes took place. This is no longer
+   necessary and is accepted only for compatibility.
 
    Same as :option:`public_flat_rw` control file option.
 

src/V3Active.cpp

@@ -562,9 +562,6 @@ class ActiveVisitor final : public VNVisitor {
         AstActive* const activep = m_namer.makeActive(nodep->fileline(), sentreep);
         activep->addStmtsp(nodep->unlinkFrBack());
     }
-    void visit(AstAlwaysPublic* nodep) override {
-        visitAlways(nodep, nodep->sentreep(), VAlwaysKwd::ALWAYS);
-    }
     void visit(AstCFunc* nodep) override { visitSenItems(nodep); }
     void visit(AstSenItem* nodep) override {
         UASSERT_OBJ(!m_walkingBody, nodep,

src/V3ActiveTop.cpp

@@ -137,9 +137,6 @@ class ActiveTopVisitor final : public VNVisitor {
     void visit(AstAssignW* nodep) override {  // LCOV_EXCL_LINE
         nodep->v3fatalSrc("Node should have been under ACTIVE");
     }
-    void visit(AstAlwaysPublic* nodep) override {  // LCOV_EXCL_LINE
-        nodep->v3fatalSrc("Node should have been under ACTIVE");
-    }
     //--------------------
     void visit(AstNodeExpr*) override {}  // Accelerate
     void visit(AstVarScope*) override {}  // Accelerate

src/V3AstNodeStmt.h

@@ -219,23 +219,6 @@ public:
 // === Concrete node types =====================================================
 
 // === AstNodeStmt ===
-class AstAlwaysPublic final : public AstNodeStmt {
-    // "Fake" sensitivity created by /*verilator public_flat_rw @(edgelist)*/
-    // Body statements are just AstVarRefs to the public signals
-    // @astgen op1 := sentreep : Optional[AstSenTree]
-    // @astgen op2 := stmtsp : List[AstNode]
-public:
-    AstAlwaysPublic(FileLine* fl, AstSenTree* sentreep, AstNode* stmtsp)
-        : ASTGEN_SUPER_AlwaysPublic(fl) {
-        this->sentreep(sentreep);
-        addStmtsp(stmtsp);
-    }
-    ASTGEN_MEMBERS_AstAlwaysPublic;
-    bool sameNode(const AstNode* /*samep*/) const override { return true; }
-    // Special accessors
-    bool isJustOneBodyStmt() const { return stmtsp() && !stmtsp()->nextp(); }
-    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == stmtsp(); }
-};
 class AstAssertCtl final : public AstNodeStmt {
     // @astgen op1 := controlTypep : AstNodeExpr
     // @astgen op2 := assertTypesp : Optional[AstNodeExpr]

src/V3Control.cpp

@@ -84,35 +84,21 @@ public:
     }
 };
 
-// Only public_flat_rw has the sensitity tree
+// List of attributes for variables
-class V3ControlVarAttr final {
+class V3ControlVar final {
-public:
+    std::vector<VAttrType> m_attrs;  // The list of attributes
-    VAttrType m_type;  // Type of attribute
-    AstSenTree* m_sentreep;  // Sensitivity tree for public_flat_rw
-    explicit V3ControlVarAttr(VAttrType type)
-        : m_type{type}
-        , m_sentreep{nullptr} {}
-    V3ControlVarAttr(VAttrType type, AstSenTree* sentreep)
-        : m_type{type}
-        , m_sentreep{sentreep} {}
-};
-
-// Overload vector with the required update function and to apply all entries
-class V3ControlVar final : public std::vector<V3ControlVarAttr> {
 public:
+    // Add new attribugte
+    void add(VAttrType attr) { m_attrs.emplace_back(attr); }
     // Update from other by copying all attributes
-    void update(const V3ControlVar& node) {
+    void update(const V3ControlVar& other) {
-        reserve(size() + node.size());
+        m_attrs.reserve(m_attrs.size() + other.m_attrs.size());
-        insert(end(), node.begin(), node.end());
+        m_attrs.insert(m_attrs.end(), other.m_attrs.begin(), other.m_attrs.end());
     }
     // Apply all attributes to the variable
-    void apply(AstVar* varp) {
+    void apply(AstVar* varp) const {
-        for (const_iterator it = begin(); it != end(); ++it) {
+        for (const VAttrType attr : m_attrs) {
-            AstNode* const newp = new AstAttrOf{varp->fileline(), it->m_type};
+            varp->addAttrsp(new AstAttrOf{varp->fileline(), attr});
-            varp->addAttrsp(newp);
-            if (it->m_type == VAttrType::VAR_PUBLIC_FLAT_RW && it->m_sentreep) {
-                newp->addNext(new AstAlwaysPublic{varp->fileline(), it->m_sentreep, nullptr});
-            }
         }
     }
 };
@@ -691,11 +677,15 @@ void V3Control::addScopeTraceOn(bool on, const string& scope, int levels) {
 
 void V3Control::addVarAttr(FileLine* fl, const string& module, const string& ftask,
                            const string& var, VAttrType attr, AstSenTree* sensep) {
-    // Semantics: sensep only if public_flat_rw
+    if (sensep) {
-    if ((attr != VAttrType::VAR_PUBLIC_FLAT_RW) && sensep) {
+        FileLine* const flp = sensep->fileline();
-        sensep->v3error("sensitivity not expected for attribute");
+        // Historical, not actually needed, only parsed for compatibility, delete it
         VL_DO_DANGLING(sensep->deleteTree(), sensep);
-        return;
+        // Used to be only accepted on public_flat_rw
+        if (attr != VAttrType::VAR_PUBLIC_FLAT_RW) {
+            flp->v3error("sensitivity not expected for attribute");
+            return;
+        }
     }
     // Semantics: Most of the attributes operate on signals
     if (var.empty()) {
@@ -723,15 +713,14 @@ void V3Control::addVarAttr(FileLine* fl, const string& module, const string& fta
             } else if (!ftask.empty()) {
                 fl->v3error("Signals inside functions/tasks cannot be marked forceable");
             } else {
-                V3ControlResolver::s().modules().at(module).vars().at(var).push_back(
+                V3ControlResolver::s().modules().at(module).vars().at(var).add(attr);
-                    V3ControlVarAttr{attr});
             }
         } else {
             V3ControlModule& mod = V3ControlResolver::s().modules().at(module);
             if (ftask.empty()) {
-                mod.vars().at(var).push_back(V3ControlVarAttr{attr, sensep});
+                mod.vars().at(var).add(attr);
             } else {
-                mod.ftasks().at(ftask).vars().at(var).push_back(V3ControlVarAttr{attr, sensep});
+                mod.ftasks().at(ftask).vars().at(var).add(attr);
             }
         }
     }

src/V3EmitCFunc.h

@@ -578,7 +578,6 @@ public:
         if (decind) ofp()->blockDec();
         puts(";\n");
     }
-    void visit(AstAlwaysPublic*) override {}
     void visit(AstAssocSel* nodep) override {
         iterateAndNextConstNull(nodep->fromp());
         putnbs(nodep, ".at(");

src/V3EmitV.cpp

@@ -142,18 +142,6 @@ class EmitVBaseVisitorConst VL_NOT_FINAL : public VNVisitorConst {
         iterateAndNextConstNull(nodep->stmtsp());
         putqs(nodep, "end\n");
     }
-    void visit(AstAlwaysPublic* nodep) override {
-        putfs(nodep, "/*verilator public_flat_rw ");
-        if (m_sentreep) {
-            iterateAndNextConstNull(m_sentreep);
-        }  // In active
-        else {
-            iterateAndNextConstNull(nodep->sentreep());
-        }
-        putqs(nodep, " ");
-        iterateAndNextConstNull(nodep->stmtsp());
-        putqs(nodep, "*/\n");
-    }
     void visit(AstNodeAssign* nodep) override {
         if (VN_IS(nodep, AssignForce)) puts("force ");
         iterateAndNextConstNull(nodep->lhsp());

src/V3Force.cpp

@@ -362,7 +362,6 @@ class ForceReplaceVisitor final : public VNVisitor {
         m_stmtp = nodep;
         iterateChildren(nodep);
     }
-    void visit(AstAlwaysPublic* nodep) override { iterateLogic(nodep); }
     void visit(AstCFunc* nodep) override { iterateLogic(nodep); }
     void visit(AstCoverToggle* nodep) override { iterateLogic(nodep); }
     void visit(AstNodeProcedure* nodep) override { iterateLogic(nodep); }

src/V3Gate.cpp

@@ -258,7 +258,6 @@ class GateBuildVisitor final : public VNVisitorConst {
         iterateChildrenConst(nodep);
     }
 
-    void visit(AstAlwaysPublic* nodep) override { iterateLogic(nodep, true, "AlwaysPublic"); }
     void visit(AstCFunc* nodep) override {  //
         iterateLogic(nodep, nodep->slow(), "C Function", "C Function");
     }

src/V3LinkParse.cpp

@@ -492,22 +492,6 @@ class LinkParseVisitor final : public VNVisitor {
         }
     }
 
-    void visit(AstAlwaysPublic* nodep) override {
-        // AlwaysPublic was attached under a var, but it's a statement that should be
-        // at the same level as the var
-        cleanFileline(nodep);
-        iterateChildren(nodep);
-        if (m_varp) {
-            nodep->unlinkFrBack();
-            AstNode::addNext<AstNode, AstNode>(m_varp, nodep);
-            // lvalue is true, because we know we have a verilator public_flat_rw
-            // but someday we may be more general
-            const bool lvalue = m_varp->isSigUserRWPublic();
-            nodep->addStmtsp(
-                new AstVarRef{nodep->fileline(), m_varp, lvalue ? VAccess::WRITE : VAccess::READ});
-        }
-    }
-
     void visit(AstDefImplicitDType* nodep) override {
         cleanFileline(nodep);
         UINFO(8, "   DEFIMPLICIT " << nodep);

src/V3OrderGraphBuilder.cpp

@@ -334,9 +334,6 @@ class OrderGraphBuilder final : public VNVisitor {
     void visit(AstAssignW* nodep) override { iterateLogic(nodep); }
 
     //--- Verilator concoctions
-    void visit(AstAlwaysPublic* nodep) override {  //
-        iterateLogic(nodep);
-    }
     void visit(AstCoverToggle* nodep) override {  //
         iterateLogic(nodep);
     }

src/V3SchedPartition.cpp

@@ -238,7 +238,6 @@ class SchedGraphBuilder final : public VNVisitor {
     void visit(AstNodeProcedure* nodep) override { visitLogic(nodep); }
     void visit(AstNodeAssign* nodep) override { visitLogic(nodep); }
     void visit(AstCoverToggle* nodep) override { visitLogic(nodep); }
-    void visit(AstAlwaysPublic* nodep) override { visitLogic(nodep); }
 
     // Pre and Post logic are handled separately
     void visit(AstAlwaysPre* nodep) override {}

src/V3Scope.cpp

@@ -227,14 +227,6 @@ class ScopeVisitor final : public VNVisitor {
         m_scopep->addBlocksp(clonep);
         iterateChildren(clonep);  // We iterate under the *clone*
     }
-    void visit(AstAlwaysPublic* nodep) override {
-        // Add to list of blocks under this scope
-        UINFO(4, "    Move " << nodep);
-        AstNode* const clonep = nodep->cloneTree(false);
-        nodep->user2p(clonep);
-        m_scopep->addBlocksp(clonep);
-        iterateChildren(clonep);  // We iterate under the *clone*
-    }
     void visit(AstCoverToggle* nodep) override {
         // Add to list of blocks under this scope
         UINFO(4, "    Move " << nodep);
@@ -365,7 +357,6 @@ class ScopeCleanupVisitor final : public VNVisitor {
     void visit(AstAssignAlias* nodep) override { movedDeleteOrIterate(nodep); }
     void visit(AstAssignVarScope* nodep) override { movedDeleteOrIterate(nodep); }
     void visit(AstAssignW* nodep) override { movedDeleteOrIterate(nodep); }
-    void visit(AstAlwaysPublic* nodep) override { movedDeleteOrIterate(nodep); }
     void visit(AstCoverToggle* nodep) override { movedDeleteOrIterate(nodep); }
     void visit(AstNodeFTask* nodep) override { movedDeleteOrIterate(nodep); }
     void visit(AstCFunc* nodep) override { movedDeleteOrIterate(nodep); }

src/V3SplitVar.cpp

@@ -213,8 +213,6 @@ struct SplitVarImpl VL_NOT_FINAL {
         AstNode* const backp = stmtp->backp();
         if (AstAlways* const ap = VN_CAST(backp, Always)) {
             insertBeginCore(ap, stmtp, modp);
-        } else if (AstAlwaysPublic* const ap = VN_CAST(backp, AlwaysPublic)) {
-            insertBeginCore(ap, stmtp, modp);
         } else if (AstInitial* const ap = VN_CAST(backp, Initial)) {
             insertBeginCore(ap, stmtp, modp);
         } else if (auto* const ap = VN_CAST(backp, Initial)) {
@@ -480,14 +478,6 @@ class SplitUnpackedVarVisitor final : public VNVisitor, public SplitVarImpl {
         for (AstNode* bodysp = nodep->stmtsp(); bodysp; bodysp = bodysp->nextp()) {
             iterate(bodysp);
         }
-    };
-    void visit(AstAlwaysPublic* nodep) override {
-        if (nodep->sentreep()) {  // When visiting sensitivity list, always is the context
-            setContextAndIterate(nodep, nodep->sentreep());
-        }
-        for (AstNode* bodysp = nodep->stmtsp(); bodysp; bodysp = bodysp->nextp()) {
-            iterate(bodysp);
-        }
     }
     void visit(AstNodeFTaskRef* nodep) override {
         const AstNodeFTask* const ftaskp = nodep->taskp();

src/verilog.y

@@ -2989,10 +2989,8 @@ netId<strp>:
 
 sigAttrScope:
                 yVL_PUBLIC_FLAT_RW_ON_SNS attr_event_control
-                                                        { AstNode* sigAttrsp = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT_RW};
+                                                        { GRAMMARP->createScopedSigAttr(VAttrType::VAR_PUBLIC_FLAT_RW);
-                                                          sigAttrsp->addNext(new AstAlwaysPublic{$1, $2, nullptr});
+                                                          v3Global.dpi(true); DEL($2); }
-                                                          GRAMMARP->setScopedSigAttr(sigAttrsp);
-                                                          v3Global.dpi(true); }
         |       yVL_PUBLIC_ON                           { GRAMMARP->createScopedSigAttr(VAttrType::VAR_PUBLIC); }
         |       yVL_PUBLIC_FLAT_ON                      { GRAMMARP->createScopedSigAttr(VAttrType::VAR_PUBLIC_FLAT); }
         |       yVL_PUBLIC_FLAT_RD_ON                   { GRAMMARP->createScopedSigAttr(VAttrType::VAR_PUBLIC_FLAT_RD); }
@@ -3018,9 +3016,7 @@ sigAttr<nodep>:
         |       yVL_PUBLIC                              { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC}; v3Global.dpi(true); }
         |       yVL_PUBLIC_FLAT                         { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT}; v3Global.dpi(true); }
         |       yVL_PUBLIC_FLAT_RD                      { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT_RD}; v3Global.dpi(true); }
-        |       yVL_PUBLIC_FLAT_RW                      { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT_RW}; v3Global.dpi(true); }
+        |       yVL_PUBLIC_FLAT_RW attr_event_controlE  { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT_RW}; v3Global.dpi(true); DEL($2); }
-        |       yVL_PUBLIC_FLAT_RW attr_event_control   { $$ = new AstAttrOf{$1, VAttrType::VAR_PUBLIC_FLAT_RW}; v3Global.dpi(true);
-                                                          $$ = $$->addNext(new AstAlwaysPublic{$1, $2, nullptr}); }
         |       yVL_ISOLATE_ASSIGNMENTS                 { $$ = new AstAttrOf{$1, VAttrType::VAR_ISOLATE_ASSIGNMENTS}; }
         |       yVL_SC_BV                               { $$ = new AstAttrOf{$1, VAttrType::VAR_SC_BV}; }
         |       yVL_SFORMAT                             { $$ = new AstAttrOf{$1, VAttrType::VAR_SFORMAT}; }

test_regress/t/t_debug_emitv.out

@@ -359,7 +359,6 @@ module Vt_debug_emitv_t;
             release sum;
         end
     end
-    /*verilator public_flat_rw @(posedge clk) pubflat*/
 endmodule
 package Vt_debug_emitv___024unit;
     class Vt_debug_emitv_Cls;

test_regress/t/t_dpi_var.v

@@ -87,12 +87,12 @@ module sub (/*AUTOARG*/
 `verilog
 
    /* verilator lint_off ASSIGNIN */
-`ifdef ATTRIBUTES
+`ifdef ATTRIBUTES // Sensitivity list accepted for backward compatibility but ignored
    input int in   /*verilator public_flat_rd*/;
    input int in_a /*verilator public_flat_rw @(posedge t.monclk)*/;
-   input int in_b /*verilator public_flat_rw @(posedge t.monclk)*/;
+   input int in_b /*verilator public_flat_rw*/;
    output int fr_a /*verilator public_flat_rw @(posedge t.monclk)*/;
-   output int fr_b /*verilator public_flat_rw @(posedge t.monclk)*/;
+   output int fr_b /*verilator public_flat_rw*/;
 `else
    input int in;
    input int in_a;

test_regress/t/t_dpi_var.vlt

@@ -8,7 +8,7 @@
 
 sformat -task "mon_scope_name" -var "formatted"
 public_flat_rd -module "sub" -var "in"
-public_flat_rw -module "sub" -var "in_a" @(posedge t.monclk)
+public_flat_rw -module "sub" -var "in_a"
 public_flat_rw -module "sub" -var "in_b" @(posedge t.monclk)
-public_flat_rw -module "sub" -var "fr_a" @(posedge t.monclk)
+public_flat_rw -module "sub" -var "fr_a"
 public_flat_rw -module "sub" -var "fr_b" @(posedge t.monclk)