Do not use function locals in SenExprBuilder (#5822)

Function locals are not safe here because we might need to split up
the generated function. V3Localize can fix them later if safe.

src/V3Sched.cpp

@@ -752,14 +752,6 @@ const TriggerKit createTriggers(AstNetlist* netlistp, AstCFunc* const initFuncp,
             funcp->stmtsp()->addHereThisAsNext(nodep);
         }
     }
-    const auto& locals = senExprBuilder.getAndClearLocals();
-    if (!locals.empty()) {
-        UASSERT_OBJ(funcp->stmtsp(), funcp,
-                    "No statements in trigger eval function, but there are locals");
-        for (AstVar* const nodep : vlstd::reverse_view(locals)) {
-            funcp->stmtsp()->addHereThisAsNext(nodep);
-        }
-    }
 
     // Add the initialization statements
     if (initialTrigsp) {

src/V3SenExprBuilder.h

@@ -31,7 +31,6 @@ class SenExprBuilder final {
     // STATE
     AstScope* const m_scopep;  // The scope
 
-    std::vector<AstVar*> m_locals;  // Trigger eval local variables
     std::vector<AstNodeStmt*> m_inits;  // Initialization statements for previous values
     std::vector<AstNodeStmt*> m_preUpdates;  // Pre update assignments
     std::vector<AstNodeStmt*> m_postUpdates;  // Post update assignments
@@ -64,6 +63,21 @@ class SenExprBuilder final {
     }
 
     // METHODS
+    AstVarScope* crateTemp(AstNodeExpr* exprp) {
+        // For readability, use the scoped signal name if the trigger is a simple AstVarRef
+        string name;
+        if (AstVarRef* const refp = VN_CAST(exprp, VarRef)) {
+            AstVarScope* const vscp = refp->varScopep();
+            name = "__" + vscp->scopep()->nameDotless() + "__" + vscp->varp()->name();
+            name = m_prevNames.get(name);
+        } else {
+            name = m_prevNames.get(exprp);
+        }
+        AstVarScope* const vscp = m_scopep->createTemp(name, exprp->dtypep());
+        vscp->varp()->isInternal(true);
+        return vscp;
+    }
+
     AstNodeExpr* getCurr(AstNodeExpr* exprp) {
         // For simple expressions like varrefs or selects, just use them directly
         if (isSimpleExpr(exprp)) return exprp->cloneTree(false);
@@ -71,15 +85,7 @@ class SenExprBuilder final {
         // Create the 'current value' variable
         FileLine* const flp = exprp->fileline();
         auto result = m_curr.emplace(*exprp, nullptr);
-        if (result.second) {
+        if (result.second) result.first->second = crateTemp(exprp);
-            AstVar* const varp
-                = new AstVar{flp, VVarType::BLOCKTEMP, m_currNames.get(exprp), exprp->dtypep()};
-            varp->funcLocal(true);
-            m_locals.push_back(varp);
-            AstVarScope* vscp = new AstVarScope{flp, m_scopep, varp};
-            m_scopep->addVarsp(vscp);
-            result.first->second = vscp;
-        }
         AstVarScope* const currp = result.first->second;
 
         // Add pre update if it does not exist yet in this round
@@ -98,28 +104,8 @@ class SenExprBuilder final {
         // Create the 'previous value' variable
         const auto pair = m_prev.emplace(*scopeExprp, nullptr);
         if (pair.second) {
-            AstVarScope* prevp;
+            AstVarScope* const prevp = crateTemp(exprp);
-            if (m_scopep->isTop()) {
-                // For readability, use the scoped signal name if the trigger is a simple AstVarRef
-                string name;
-                if (AstVarRef* const refp = VN_CAST(exprp, VarRef)) {
-                    AstVarScope* const vscp = refp->varScopep();
-                    name = "__" + vscp->scopep()->nameDotless() + "__" + vscp->varp()->name();
-                    name = m_prevNames.get(name);
-                } else {
-                    name = m_prevNames.get(exprp);
-                }
-                prevp = m_scopep->createTemp(name, exprp->dtypep());
-            } else {
-                AstVar* const varp = new AstVar{flp, VVarType::BLOCKTEMP, m_prevNames.get(exprp),
-                                                exprp->dtypep()};
-                varp->funcLocal(true);
-                m_locals.push_back(varp);
-                prevp = new AstVarScope{flp, m_scopep, varp};
-                m_scopep->addVarsp(prevp);
-            }
             pair.first->second = prevp;
-
             // Add the initializer init
             AstAssign* const initp = new AstAssign{flp, new AstVarRef{flp, prevp, VAccess::WRITE},
                                                    exprp->cloneTree(false)};
@@ -231,13 +217,6 @@ public:
 
     std::vector<AstNodeStmt*> getAndClearInits() { return std::move(m_inits); }
 
-    std::vector<AstVar*> getAndClearLocals() {
-        // With m_locals empty, m_prev and m_curr are no longer valid
-        m_prev.clear();
-        m_curr.clear();
-        return std::move(m_locals);
-    }
-
     std::vector<AstNodeStmt*> getAndClearPreUpdates() {
         m_hasPreUpdate.clear();
         return std::move(m_preUpdates);

src/V3Timing.cpp

@@ -933,10 +933,7 @@ class TimingControlVisitor final : public VNVisitor {
             UASSERT_OBJ(m_senExprBuilderp, nodep, "No SenExprBuilder for this scope");
             auto* const assignp = new AstAssign{flp, new AstVarRef{flp, trigvscp, VAccess::WRITE},
                                                 m_senExprBuilderp->build(sensesp).first};
-            // Put all the locals and inits before the trigger eval loop
+            // Put all and inits before the trigger eval loop
-            for (AstVar* const varp : m_senExprBuilderp->getAndClearLocals()) {
-                nodep->addHereThisAsNext(varp);
-            }
             for (AstNodeStmt* const stmtp : m_senExprBuilderp->getAndClearInits()) {
                 nodep->addHereThisAsNext(stmtp);
             }

test_regress/t/t_flag_csplit.v

@@ -53,4 +53,9 @@ module sub (input clk, input [31:0] i, output [31:0] z);
      z_tmp <= i+1+$c("0");  // $c so doesn't optimize away
 
    assign z = z_tmp;
+
+   always @(posedge z_tmp == 32'b11) begin
+     $display("%m z_tmp[0]: %d", z_tmp);
+   end
+
 endmodule

test_regress/t/t_flag_csplit_groups.py

@@ -124,7 +124,7 @@ test.file_grep_not(test.obj_dir + "/" + test.vm_prefix + "_classes.mk", "vm_clas
 test.file_grep_not(test.obj_dir + "/" + test.vm_prefix + "_classes.mk", "vm_classes_2")
 
 # Check combine count
-test.file_grep(test.stats, r'Node count, CFILE + (\d+)', (191 if test.vltmt else 173))
+test.file_grep(test.stats, r'Node count, CFILE + (\d+)', (232 if test.vltmt else 212))
 test.file_grep(test.stats, r'Makefile targets, VM_CLASSES_FAST + (\d+)', 2)
 test.file_grep(test.stats, r'Makefile targets, VM_CLASSES_SLOW + (\d+)', 2)