From 08c8b0d7d655d2c8afe91114818e2e943bc8a601 Mon Sep 17 00:00:00 2001
From: Wilson Snyder <wsnyder@wsnyder.org>
Date: Mon, 13 Sep 2021 15:52:53 -0400
Subject: [PATCH] Fix removing if statement with side effect in condition
 (#3131).

---
 Changes                         |  1 +
 src/V3AstNodes.h                |  1 +
 src/V3Const.cpp                 | 20 ++++++++++++++++----
 test_regress/t/t_sys_plusargs.v |  5 +++++
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/Changes b/Changes
index cf419d12e..d3dd8f2a6 100644
--- a/Changes
+++ b/Changes
@@ -14,6 +14,7 @@ Verilator 4.213 devel
 * Include processor information in verilator_gantt data file.
 * Fix verilator_profcfunc profile accounting (#3115).
 * Fix display has no time units on class function (#3116). [Damien Pretet]
+* Fix removing if statement with side effect in condition (#3131). [Alexander Grobman]
 
 
 Verilator 4.212 2021-09-01
diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h
index da6da2a01..d9351f101 100644
--- a/src/V3AstNodes.h
+++ b/src/V3AstNodes.h
@@ -4436,6 +4436,7 @@ public:
     virtual string emitC() override { V3ERROR_NA_RETURN(""); }
     virtual bool isGateOptimizable() const override { return false; }
     virtual bool isPredictOptimizable() const override { return false; }
+    virtual bool isPure() const override { return !outp(); }
     virtual bool cleanOut() const override { return true; }
     virtual bool same(const AstNode* samep) const override { return true; }
     AstNode* searchp() const { return op1p(); }  // op1 = Search expression
diff --git a/src/V3Const.cpp b/src/V3Const.cpp
index 6c279f8ad..5e7984bdf 100644
--- a/src/V3Const.cpp
+++ b/src/V3Const.cpp
@@ -1312,6 +1312,15 @@ private:
         // but for now can disable en-mass until V3Purify takes effect.
         return m_doShort || VN_IS(nodep, VarRef) || VN_IS(nodep, Const);
     }
+    bool isTreePureRecurse(AstNode* nodep) {
+        // Should memoize this if call commonly
+        if (!nodep->isPure()) return false;
+        if (nodep->op1p() && !isTreePureRecurse(nodep->op1p())) return false;
+        if (nodep->op2p() && !isTreePureRecurse(nodep->op2p())) return false;
+        if (nodep->op3p() && !isTreePureRecurse(nodep->op3p())) return false;
+        if (nodep->op4p() && !isTreePureRecurse(nodep->op4p())) return false;
+        return true;
+    }
 
     // Extraction checks
     bool warnSelect(AstSel* nodep) {
@@ -2783,10 +2792,13 @@ private:
                 }
                 VL_DO_DANGLING(nodep->deleteTree(), nodep);
             } else if (!afterComment(nodep->ifsp()) && !afterComment(nodep->elsesp())) {
-                // Empty block, remove it
-                // Note if we support more C++ then there might be side
-                // effects in the condition itself
-                VL_DO_DANGLING(nodep->unlinkFrBack()->deleteTree(), nodep);
+                if (!isTreePureRecurse(nodep->condp())) {
+                    // Condition has side effect - leave - perhaps in
+                    // future simplify to remove all but side effect terms
+                } else {
+                    // Empty block, remove it
+                    VL_DO_DANGLING(nodep->unlinkFrBack()->deleteTree(), nodep);
+                }
             } else if (!afterComment(nodep->ifsp())) {
                 UINFO(4, "IF({x}) nullptr {...} => IF(NOT{x}}: " << nodep << endl);
                 AstNode* condp = nodep->condp();
diff --git a/test_regress/t/t_sys_plusargs.v b/test_regress/t/t_sys_plusargs.v
index 015ccfb2c..73600e6cd 100644
--- a/test_regress/t/t_sys_plusargs.v
+++ b/test_regress/t/t_sys_plusargs.v
@@ -103,6 +103,11 @@ module t;
       $display("i='%d'",p_i);
       if (p_i !== 32'd1234) $stop;
 
+      // bug3131 - really "if" side effect test
+      p_i = 0;
+      if ($value$plusargs("INT=%d", p_i)) ;
+      if (p_i !== 32'd1234) $stop;
+
       $write("*-* All Finished *-*\n");
       $finish;
    end