Optimize empty expression statements (#4544).

Changes

@@ -19,6 +19,7 @@ Verilator 5.017 devel
 * Support resizing function call inout arguments (#4467).
 * Support converting parameters inside modules to localparams (#4511). [Anthony Donlon]
 * Change lint_off to not propagate upwards to files including where the lint_off is.
+* Optimize empty expression statements (#4544).
 * Fix conversion of impure logical expressions to bit expressions (#487 partial) (#4437). [Ryszard Rozak, Antmicro Ltd.]
 * Fix enum functions in localparams (#3999). [Andrew Nolte]
 * Fix passing arguments by reference (#3385 partial) (#4489). [Ryszard Rozak, Antmicro Ltd.]

src/V3Ast.cpp

@@ -33,6 +33,8 @@ VL_DEFINE_DEBUG_FUNCTIONS;
 //======================================================================
 // Statics
 
+uint32_t VIsCached::s_cachedCntGbl = 1;
+
 uint64_t AstNode::s_editCntLast = 0;
 uint64_t AstNode::s_editCntGbl = 0;  // Hot cache line
 

src/V3Ast.h

@@ -169,15 +169,31 @@ inline std::ostream& operator<<(std::ostream& os, const VLifetime& rhs) {
 
 class VIsCached final {
     // Used in some nodes to cache results of boolean methods
+    // If cachedCnt == 0, not cached
+    // else if cachedCnt == s_cachedCntGbl, then m_state is if cached
+    uint32_t m_cachedCnt : 31;  // Mark of when cache was computed
+    uint32_t m_state : 1;
+    static uint32_t s_cachedCntGbl;  // Global computed count
+    static constexpr uint32_t MAX_CNT = (1UL << 31) - 1;  // Max for m_cachedCnt
+
 public:
-    enum en : uint8_t { NOT_CACHED, YES, NO };
-    enum en m_e;
     VIsCached()
-        : m_e{NOT_CACHED} {}
-    bool isCached() const { return m_e != NOT_CACHED; }
-    bool get() const { return m_e == YES; }
-    void set(bool flag) { m_e = flag ? YES : NO; }
-    void clearCache() { m_e = NOT_CACHED; }
+        : m_cachedCnt{0}
+        , m_state{0} {}
+    bool isCached() const { return m_cachedCnt == s_cachedCntGbl; }
+    bool get() const { return m_state; }
+    void set(bool flag) {
+        m_cachedCnt = s_cachedCntGbl;
+        m_state = flag;
+    }
+    void clearCache() {
+        m_cachedCnt = 0;
+        m_state = 0;
+    }
+    static void clearCacheTree() {
+        ++s_cachedCntGbl;
+        UASSERT_STATIC(s_cachedCntGbl < MAX_CNT, "Overflow of cache counting");
+    }
 };
 
 // ######################################################################

src/V3AstNodeExpr.h

@@ -1176,7 +1176,10 @@ public:
     string emitVerilog() override { V3ERROR_NA_RETURN(""); }
     string emitC() override { V3ERROR_NA_RETURN(""); }
     bool cleanOut() const override { return true; }
-    bool isPure() override { return false; }
+    bool isPure() override {
+        if (AstNode::afterCommentp(stmtsp())) return false;
+        return resultp()->isPure();
+    }
     bool same(const AstNode*) const override { return true; }
 };
 class AstFError final : public AstNodeExpr {

src/V3Const.cpp

@@ -2689,6 +2689,15 @@ private:
                            << nodep->varp()->prettyNameQ());
         }
     }
+    void visit(AstExprStmt* nodep) override {
+        iterateChildren(nodep);
+        if (!AstNode::afterCommentp(nodep->stmtsp())) {
+            UINFO(8, "ExprStmt(...) " << nodep << " " << nodep->resultp() << endl);
+            nodep->replaceWith(nodep->resultp()->unlinkFrBack());
+            VL_DO_DANGLING(nodep->deleteTree(), nodep);
+            // Removing the ExprStmt might have made something impure above now pure
+        }
+    }
     void visit(AstEnumItemRef* nodep) override {
         iterateChildren(nodep);
         UASSERT_OBJ(nodep->itemp(), nodep, "Not linked");
@@ -3718,6 +3727,7 @@ public:
     }
 
     AstNode* mainAcceptEdit(AstNode* nodep) {
+        VIsCached::clearCacheTree();  // Avoid using any stale isPure
         // Operate starting at a random place
         return iterateSubtreeReturnEdits(nodep);
     }

src/V3Dead.cpp

@@ -533,6 +533,7 @@ public:
 
         // We may have removed some datatypes, cleanup
         nodep->typeTablep()->repairCache();
+        VIsCached::clearCacheTree();  // Removing assignments may affect isPure
     }
     ~DeadVisitor() override = default;
 };

src/V3Life.cpp

@@ -517,5 +517,6 @@ void V3Life::lifeAll(AstNetlist* nodep) {
         LifeState state;
         LifeTopVisitor{nodep, &state};
     }  // Destruct before checking
+    VIsCached::clearCacheTree();  // Removing assignments may affect isPure
     V3Global::dumpCheckGlobalTree("life", 0, dumpTreeLevel() >= 3);
 }

test_regress/t/t_func_real_exprstmt.pl

@@ -11,8 +11,14 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 scenarios(simulator => 1);
 
 compile(
+    verilator_flags2 => ["--stats"],
     );
 
+if ($Self->{vlt_all}) {
+    # Check no EXPRSTMTs in final output - should get optimized away
+    file_grep_not($Self->{stats}, qr/Node count, EXPRSTMT/i);
+}
+
 execute(
     check_finished => 1,
     );