Don't move function calls before the expression (#4413)

src/V3AstNodeExpr.h

@@ -1110,6 +1110,7 @@ public:
     string emitVerilog() override { V3ERROR_NA_RETURN(""); }
     string emitC() override { V3ERROR_NA_RETURN(""); }
     bool cleanOut() const override { return true; }
+    bool isPure() const override { return false; }
     bool same(const AstNode*) const override { return true; }
 };
 class AstFError final : public AstNodeExpr {

src/V3Dead.cpp

@@ -288,7 +288,6 @@ private:
             VL_RESTORER(m_sideEffect);
             m_inAssign = true;
             m_sideEffect = false;
-            if (assignInAssign) m_sideEffect = true;
             iterateAndNextNull(nodep->rhsp());
             checkAll(nodep);
             // Has to be direct assignment without any EXTRACTing.

src/V3Delayed.cpp

@@ -551,7 +551,9 @@ private:
             }
             if (!lhsp->backp()) VL_DO_DANGLING(pushDeletep(lhsp), lhsp);
         } else {
-            iterateChildren(nodep);
+            iterate(nodep->lhsp());
+            m_inDly = false;
+            iterate(nodep->rhsp());
         }
     }
 
@@ -623,9 +625,9 @@ private:
         m_inLoop = true;
         iterateChildren(nodep);
     }
-    void visit(AstNodeAssign* nodep) override {
+    void visit(AstExprStmt* nodep) override {
         VL_RESTORER(m_inDly);
-        // Restoring is needed in nested assignments, like a <= (x = y);
+        // Restoring is needed, because AstExprStmt may contain assignments
         m_inDly = false;
         iterateChildren(nodep);
     }

src/V3EmitCFunc.h

@@ -419,7 +419,8 @@ public:
                    && !VN_IS(nodep->rhsp(), AssocSel)  //
                    && !VN_IS(nodep->rhsp(), MemberSel)  //
                    && !VN_IS(nodep->rhsp(), StructSel)  //
-                   && !VN_IS(nodep->rhsp(), ArraySel)) {
+                   && !VN_IS(nodep->rhsp(), ArraySel)  //
+                   && !VN_IS(nodep->rhsp(), ExprStmt)) {
             // Wide functions assign into the array directly, don't need separate assign statement
             m_wideTempRefp = VN_AS(nodep->lhsp(), VarRef);
             paren = false;

src/V3EmitV.cpp

@@ -664,6 +664,12 @@ class EmitVBaseVisitorConst VL_NOT_FINAL : public EmitCBaseVisitorConst {
         iterateAndNextConstNull(nodep->pinsp());
         puts(")");
     }
+    void visit(AstCCall* nodep) override {
+        puts(nodep->funcp()->name());
+        puts("(");
+        iterateAndNextConstNull(nodep->argsp());
+        puts(")");
+    }
     void visit(AstArg* nodep) override { iterateAndNextConstNull(nodep->exprp()); }
     void visit(AstPrintTimeScale* nodep) override {
         puts(nodep->verilogKwd());

src/V3Gate.cpp

@@ -331,7 +331,6 @@ private:
     AstActive* m_activep = nullptr;  // Current active
     bool m_activeReducible = true;  // Is activation block reducible?
     bool m_inSenItem = false;  // Underneath AstSenItem; any varrefs are clocks
-    bool m_inExprStmt = false;  // Underneath ExprStmt; don't optimize LHS vars
     bool m_inSlow = false;  // Inside a slow structure
     std::vector<AstNode*> m_optimized;  // Logic blocks optimized
 
@@ -497,10 +496,6 @@ private:
             // the weight will increase
             if (nodep->access().isWriteOrRW()) {
                 new V3GraphEdge{&m_graph, m_logicVertexp, vvertexp, 1};
-                if (m_inExprStmt) {
-                    m_logicVertexp->clearReducibleAndDedupable("LHS var in ExprStmt");
-                    m_logicVertexp->setConsumed("LHS var in ExprStmt");
-                }
             }
             if (nodep->access().isReadOrRW()) {
                 new V3GraphEdge{&m_graph, vvertexp, m_logicVertexp, 1};
@@ -516,11 +511,6 @@ private:
         iterateNewStmt(nodep, "User C Function", "User C Function");
     }
     void visit(AstClocking* nodep) override { iterateNewStmt(nodep, nullptr, nullptr); }
-    void visit(AstExprStmt* nodep) override {
-        VL_RESTORER(m_inExprStmt);
-        m_inExprStmt = true;
-        iterateChildren(nodep);
-    }
     void visit(AstSenItem* nodep) override {
         VL_RESTORER(m_inSenItem);
         m_inSenItem = true;
@@ -905,6 +895,7 @@ class GateDedupeVarVisitor final : public VNVisitor {
     //   (Note, the IF must be the only node under the always,
     //    and the assign must be the only node under the if, other than the ifcond)
     // Any other ordering or node type, except for an AstComment, makes it not dedupable
+    // AstExprStmt in the subtree of a node also makes the node not dedupable.
 private:
     // STATE
     GateDedupeHash m_ghash;  // Hash used to find dupes of rhs of assign
@@ -920,6 +911,7 @@ private:
             // non-blocking statements, but erring on side of caution here
             if (!m_assignp) {
                 m_assignp = assignp;
+                m_dedupable = !assignp->exists([&](AstExprStmt*) { return true; });
             } else {
                 m_dedupable = false;
             }
@@ -944,6 +936,7 @@ private:
             if (m_always && !m_ifCondp && !ifp->elsesp()) {
                 // we're under an always, this is the first IF, and there's no else
                 m_ifCondp = ifp->condp();
+                m_dedupable = !m_ifCondp->exists([&](AstExprStmt*) { return true; });
                 iterateAndNextNull(ifp->thensp());
             } else {
                 m_dedupable = false;

src/V3Premit.cpp

@@ -54,11 +54,11 @@ private:
     const VNUser2InUse m_inuser2;
 
     // STATE - across all visitors
-    V3UniqueNames m_tempNames;  // For generating unique temporary variable names
     VDouble0 m_extractedToConstPool;  // Statistic tracking
 
     // STATE - for current visit position (use VL_RESTORER)
     AstCFunc* m_cfuncp = nullptr;  // Current block
+    int m_tmpVarCnt = 0;  // Number of temporary variables created inside a function
     AstNode* m_stmtp = nullptr;  // Current statement
     AstCCall* m_callp = nullptr;  // Current AstCCall
     AstWhile* m_inWhilep = nullptr;  // Inside while loop, special statement additions
@@ -138,7 +138,8 @@ private:
             ++m_extractedToConstPool;
         } else {
             // Keep as local temporary. Name based on hash of node for output stability.
-            varp = new AstVar{fl, VVarType::STMTTEMP, m_tempNames.get(nodep), nodep->dtypep()};
+            varp = new AstVar{fl, VVarType::STMTTEMP, "__Vtemp_" + cvtToStr(++m_tmpVarCnt),
+                              nodep->dtypep()};
             m_cfuncp->addInitsp(varp);
             // Put assignment before the referencing statement
             insertBeforeStmt(new AstAssign{fl, new AstVarRef{fl, varp, VAccess::WRITE}, nodep});
@@ -158,8 +159,9 @@ private:
     }
     void visit(AstCFunc* nodep) override {
         VL_RESTORER(m_cfuncp);
+        VL_RESTORER(m_tmpVarCnt);
         m_cfuncp = nodep;
-        m_tempNames.reset();
+        m_tmpVarCnt = 0;
         iterateChildren(nodep);
     }
 
@@ -391,10 +393,7 @@ private:
 
 public:
     // CONSTRUCTORS
-    explicit PremitVisitor(AstNetlist* nodep)
+    explicit PremitVisitor(AstNetlist* nodep) { iterate(nodep); }
-        : m_tempNames{"__Vtemp"} {
-        iterate(nodep);
-    }
     ~PremitVisitor() override {
         V3Stats::addStat("Optimizations, Prelim extracted value to ConstPool",
                          m_extractedToConstPool);

src/V3Reloop.cpp

@@ -253,6 +253,7 @@ private:
         m_mgIndexHi = lindex;
         UINFO(9, "Start merge i=" << lindex << " o=" << m_mgOffset << nodep << endl);
     }
+    void visit(AstExprStmt* nodep) override { iterateChildren(nodep); }
     //--------------------
     void visit(AstVar*) override {}  // Accelerate
     void visit(AstNodeExpr*) override {}  // Accelerate

src/V3SchedTiming.cpp

@@ -236,6 +236,7 @@ TimingKit prepareTiming(AstNetlist* const netlistp) {
                 m_writtenBySuspendable.push_back(nodep->varScopep());
             }
         }
+        void visit(AstExprStmt* nodep) override { iterateChildren(nodep); }
 
         //--------------------
         void visit(AstNodeExpr*) override {}  // Accelerate
@@ -405,6 +406,7 @@ void transformForks(AstNetlist* const netlistp) {
             if (nodep->funcp()->needProcess()) m_beginNeedProcess = true;
             iterateChildrenConst(nodep);
         }
+        void visit(AstExprStmt* nodep) override { iterateChildren(nodep); }
 
         //--------------------
         void visit(AstNodeExpr*) override {}  // Accelerate

src/V3Task.cpp

@@ -1420,18 +1420,27 @@ private:
             beginp = createInlinedFTask(nodep, namePrefix, outvscp);
         }
         // Replace the ref
-        AstNode* const visitp = insertBeforeStmt(nodep, beginp);
+        AstNode* visitp = nullptr;
 
         if (VN_IS(nodep, New)) {
+            visitp = insertBeforeStmt(nodep, beginp);
             UASSERT_OBJ(cnewp, nodep, "didn't create cnew for new");
             nodep->replaceWith(cnewp);
             VL_DO_DANGLING(nodep->deleteTree(), nodep);
+        } else if (VN_IS(nodep->backp(), NodeAssign)) {
+            UASSERT_OBJ(nodep->taskp()->isFunction(), nodep, "func reference to non-function");
+            visitp = insertBeforeStmt(nodep, beginp);
+            AstVarRef* const outrefp = new AstVarRef{nodep->fileline(), outvscp, VAccess::READ};
+            nodep->replaceWith(outrefp);
+            VL_DO_DANGLING(nodep->deleteTree(), nodep);
         } else if (!VN_IS(nodep->backp(), StmtExpr)) {
             UASSERT_OBJ(nodep->taskp()->isFunction(), nodep, "func reference to non-function");
             AstVarRef* const outrefp = new AstVarRef{nodep->fileline(), outvscp, VAccess::READ};
-            nodep->replaceWith(outrefp);
+            beginp = new AstExprStmt{nodep->fileline(), beginp, outrefp};
+            nodep->replaceWith(beginp);
             VL_DO_DANGLING(nodep->deleteTree(), nodep);
         } else {
+            visitp = insertBeforeStmt(nodep, beginp);
             if (nodep->taskp()->isFunction()) {
                 nodep->v3warn(
                     IGNOREDRETURN,

src/V3Timing.cpp

@@ -550,7 +550,10 @@ private:
             ss << '"';
             V3EmitV::verilogForTree(sensesp, ss);
             ss << '"';
-            auto* const commentp = new AstCExpr{sensesp->fileline(), ss.str(), 0};
+            // possibly a multiline string
+            std::string comment = ss.str();
+            std::replace(comment.begin(), comment.end(), '\n', ' ');
+            AstCExpr* const commentp = new AstCExpr{sensesp->fileline(), comment, 0};
             commentp->dtypeSetString();
             sensesp->user2p(commentp);
             return commentp;

test_regress/t/t_dpi_shortcircuit.out

@@ -10,8 +10,6 @@
 %Error: Line 136: Bad result, got=0 expect=1
 %Error: Line 148: Bad result, got=1 expect=0
 %Error: Line 152: Bad result, got=1 expect=0
-%Error: Line 166: Bad result, got=1 expect=0
-%Error: Line 170: Bad result, got=1 expect=0
 %Error: Line 179: Bad result, got=0 expect=1
 %Error: Line 219: Bad result, got=64 expect=32
 %Error: Line 220: Bad result, got=64 expect=16

test_regress/t/t_unopt_combo_isolate.pl

@@ -18,7 +18,7 @@ compile(
     );
 
 if ($Self->{vlt_all}) {
-    file_grep($Self->{stats}, qr/Optimizations, isolate_assignments blocks\s+5/i);
+    file_grep($Self->{stats}, qr/Optimizations, isolate_assignments blocks\s+3/i);
     file_grep("$out_filename", qr/\<var loc="e,23,.*?" name="t.b" dtype_id="\d+" vartype="logic" origName="b" isolate_assignments="true"\/\>/i);
     file_grep("$out_filename", qr/\<var loc="e,99,.*?" name="__Vfunc_t.file.get_31_16__0__Vfuncout" dtype_id="\d+" vartype="logic" origName="__Vfunc_t__DOT__file__DOT__get_31_16__0__Vfuncout" isolate_assignments="true"\/\>/i);
     file_grep("$out_filename", qr/\<var loc="e,100,.*?" name="__Vfunc_t.file.get_31_16__0__t_crc" dtype_id="\d+" vartype="logic" origName="__Vfunc_t__DOT__file__DOT__get_31_16__0__t_crc" isolate_assignments="true"\/\>/i);

test_regress/t/t_unopt_combo_isolate_vlt.pl

@@ -18,7 +18,7 @@ compile(
     );
 
 if ($Self->{vlt_all}) {
-    file_grep($Self->{stats}, qr/Optimizations, isolate_assignments blocks\s+5/i);
+    file_grep($Self->{stats}, qr/Optimizations, isolate_assignments blocks\s+3/i);
     file_grep("$out_filename", qr/\<var loc="f,23,.*?" name="t.b" dtype_id="\d+" vartype="logic" origName="b" isolate_assignments="true"\/\>/i);
     file_grep("$out_filename", qr/\<var loc="f,104,.*?" name="__Vfunc_t.file.get_31_16__0__Vfuncout" dtype_id="\d+" vartype="logic" origName="__Vfunc_t__DOT__file__DOT__get_31_16__0__Vfuncout" isolate_assignments="true"\/\>/i);
     file_grep("$out_filename", qr/\<var loc="f,105,.*?" name="__Vfunc_t.file.get_31_16__0__t_crc" dtype_id="\d+" vartype="logic" origName="__Vfunc_t__DOT__file__DOT__get_31_16__0__t_crc" isolate_assignments="true"\/\>/i);

test_regress/t/t_xml_debugcheck.out

@@ -61,7 +61,7 @@
         <creset loc="d,51,17,51,18">
           <varref loc="d,51,17,51,18" name="t.unnamedblk1.e" dtype_id="2"/>
         </creset>
-        <var loc="d,48,123,48,127" name="__Vtemp_h########__0" dtype_id="10" vartype="string" origName="__Vtemp_h########__0"/>
+        <var loc="d,48,123,48,127" name="__Vtemp_1" dtype_id="10" vartype="string" origName="__Vtemp_1"/>
         <assign loc="d,31,9,31,10" dtype_id="11">
           <const loc="d,31,11,31,14" name="4&apos;h3" dtype_id="11"/>
           <varref loc="d,31,7,31,8" name="t.e" dtype_id="11"/>
@@ -510,11 +510,11 @@
                   </ccast>
                 </and>
               </arraysel>
-              <varref loc="d,48,123,48,127" name="__Vtemp_h########__0" dtype_id="10"/>
+              <varref loc="d,48,123,48,127" name="__Vtemp_1" dtype_id="10"/>
             </assign>
             <display loc="d,48,42,48,48" displaytype="$write">
               <sformatf loc="d,48,42,48,48" name="%%Error: t/t_enum_type_methods.v:48:  got=&apos;%@&apos; exp=&apos;E03&apos;&#10;" dtype_id="10">
-                <varref loc="d,48,123,48,127" name="__Vtemp_h########__0" dtype_id="10"/>
+                <varref loc="d,48,123,48,127" name="__Vtemp_1" dtype_id="10"/>
               </sformatf>
             </display>
             <stop loc="d,48,140,48,145"/>
@@ -700,9 +700,9 @@
         <creset loc="d,23,9,23,10">
           <varref loc="d,23,9,23,10" name="__Vdly__t.e" dtype_id="2"/>
         </creset>
-        <var loc="d,67,126,67,130" name="__Vtemp_h########__0" dtype_id="10" vartype="string" origName="__Vtemp_h########__0"/>
+        <var loc="d,67,126,67,130" name="__Vtemp_1" dtype_id="10" vartype="string" origName="__Vtemp_1"/>
-        <var loc="d,77,126,77,130" name="__Vtemp_h########__1" dtype_id="10" vartype="string" origName="__Vtemp_h########__1"/>
+        <var loc="d,77,126,77,130" name="__Vtemp_2" dtype_id="10" vartype="string" origName="__Vtemp_2"/>
-        <var loc="d,87,126,87,130" name="__Vtemp_h########__2" dtype_id="10" vartype="string" origName="__Vtemp_h########__2"/>
+        <var loc="d,87,126,87,130" name="__Vtemp_3" dtype_id="10" vartype="string" origName="__Vtemp_3"/>
         <assignpre loc="d,64,10,64,11" dtype_id="11">
           <varref loc="d,64,10,64,11" name="t.e" dtype_id="11"/>
           <varref loc="d,64,10,64,11" name="__Vdly__t.e" dtype_id="11"/>
@@ -762,11 +762,11 @@
                           </ccast>
                         </and>
                       </arraysel>
-                      <varref loc="d,67,126,67,130" name="__Vtemp_h########__0" dtype_id="10"/>
+                      <varref loc="d,67,126,67,130" name="__Vtemp_1" dtype_id="10"/>
                     </assign>
                     <display loc="d,67,45,67,51" displaytype="$write">
                       <sformatf loc="d,67,45,67,51" name="%%Error: t/t_enum_type_methods.v:67:  got=&apos;%@&apos; exp=&apos;E01&apos;&#10;" dtype_id="10">
-                        <varref loc="d,67,126,67,130" name="__Vtemp_h########__0" dtype_id="10"/>
+                        <varref loc="d,67,126,67,130" name="__Vtemp_1" dtype_id="10"/>
                       </sformatf>
                     </display>
                     <stop loc="d,67,143,67,148"/>
@@ -1012,11 +1012,11 @@
                               </ccast>
                             </and>
                           </arraysel>
-                          <varref loc="d,77,126,77,130" name="__Vtemp_h########__1" dtype_id="10"/>
+                          <varref loc="d,77,126,77,130" name="__Vtemp_2" dtype_id="10"/>
                         </assign>
                         <display loc="d,77,45,77,51" displaytype="$write">
                           <sformatf loc="d,77,45,77,51" name="%%Error: t/t_enum_type_methods.v:77:  got=&apos;%@&apos; exp=&apos;E03&apos;&#10;" dtype_id="10">
-                            <varref loc="d,77,126,77,130" name="__Vtemp_h########__1" dtype_id="10"/>
+                            <varref loc="d,77,126,77,130" name="__Vtemp_2" dtype_id="10"/>
                           </sformatf>
                         </display>
                         <stop loc="d,77,143,77,148"/>
@@ -1262,11 +1262,11 @@
                                   </ccast>
                                 </and>
                               </arraysel>
-                              <varref loc="d,87,126,87,130" name="__Vtemp_h########__2" dtype_id="10"/>
+                              <varref loc="d,87,126,87,130" name="__Vtemp_3" dtype_id="10"/>
                             </assign>
                             <display loc="d,87,45,87,51" displaytype="$write">
                               <sformatf loc="d,87,45,87,51" name="%%Error: t/t_enum_type_methods.v:87:  got=&apos;%@&apos; exp=&apos;E04&apos;&#10;" dtype_id="10">
-                                <varref loc="d,87,126,87,130" name="__Vtemp_h########__2" dtype_id="10"/>
+                                <varref loc="d,87,126,87,130" name="__Vtemp_3" dtype_id="10"/>
                               </sformatf>
                             </display>
                             <stop loc="d,87,143,87,148"/>