From 40bdb080d82c30ef985491abd6336afda6c19251 Mon Sep 17 00:00:00 2001
From: Hideto Ueno <uenoku.tokotoko@gmail.com>
Date: Wed, 12 Apr 2023 00:56:59 +0900
Subject: [PATCH] [FIRRTL][IMDCE] Remove dead Invalid Value op (#4996)

We've changed the modeling of InvalidValueOp to have side-effect so
it's necessary to handle invalid values specially. Since InvalidValueOp has
MemAlloc side-effects trait, this PR allows dead operations with
known side-effects(Alloc/Read) to be removed.
---
 lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp | 15 ++++++++++++---
 test/Dialect/FIRRTL/imdce.mlir                   |  6 ++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp
index 40714e84eee7..2ce6c90076ce 100644
--- a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp
+++ b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp
@@ -11,6 +11,7 @@
 #include "circt/Dialect/FIRRTL/Passes.h"
 #include "mlir/IR/ImplicitLocOpBuilder.h"
 #include "mlir/IR/Threading.h"
+#include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/TinyPtrVector.h"
@@ -21,6 +22,13 @@
 using namespace circt;
 using namespace firrtl;
 
+// Return true if this op has side-effects except for alloc and read.
+static bool hasUnknownSideEffect(Operation *op) {
+  return !(mlir::isMemoryEffectFree(op) ||
+           mlir::hasSingleEffect<mlir::MemoryEffects::Allocate>(op) ||
+           mlir::hasSingleEffect<mlir::MemoryEffects::Read>(op));
+}
+
 /// Return true if this is a wire or a register or a node.
 static bool isDeclaration(Operation *op) {
   return isa<WireOp, RegResetOp, RegOp, NodeOp, MemOp>(op);
@@ -202,7 +210,7 @@ void IMDeadCodeElimPass::markBlockExecutable(Block *block) {
     else if (isa<FConnectLike>(op))
       // Skip connect op.
       continue;
-    else if (!mlir::isMemoryEffectFree(&op))
+    else if (hasUnknownSideEffect(&op))
       markUnknownSideEffectOp(&op);
 
     // TODO: Handle attach etc.
@@ -389,8 +397,9 @@ void IMDeadCodeElimPass::rewriteModuleBody(FModuleOp module) {
       continue;
     }
 
-    // Delete dead wires, regs and nodes.
-    if (isDeclaration(&op) && isAssumedDead(&op)) {
+    // Delete dead wires, regs, nodes and alloc/read ops.
+    if ((isDeclaration(&op) || !hasUnknownSideEffect(&op)) &&
+        isAssumedDead(&op)) {
       LLVM_DEBUG(llvm::dbgs() << "DEAD: " << op << "\n";);
       assert(op.use_empty() && "users should be already removed");
       op.erase();
diff --git a/test/Dialect/FIRRTL/imdce.mlir b/test/Dialect/FIRRTL/imdce.mlir
index 529a911c952a..3bdc0d0ce9e4 100644
--- a/test/Dialect/FIRRTL/imdce.mlir
+++ b/test/Dialect/FIRRTL/imdce.mlir
@@ -321,6 +321,10 @@ firrtl.circuit "DeadInputPort"  {
 // -----
 
 firrtl.circuit "DeleteInstance" {
+  // CHECK-NOT: @InvalidValue
+  firrtl.module private @InvalidValue() {
+      %invalid_ui289 = firrtl.invalidvalue : !firrtl.uint<289>
+  }
   firrtl.module private @SideEffect1(in %a: !firrtl.uint<1>, in %clock: !firrtl.clock) {
     firrtl.printf %clock, %a, "foo"  : !firrtl.clock, !firrtl.uint<1>
   }
@@ -334,6 +338,8 @@ firrtl.circuit "DeleteInstance" {
   }
   // CHECK-LABEL: DeleteInstance
   firrtl.module @DeleteInstance(in %a: !firrtl.uint<1>, in %clock: !firrtl.clock, out %b: !firrtl.uint<1>) {
+    // CHECK-NOT: inv
+    firrtl.instance inv @InvalidValue()
     // CHECK-NOT: p1
     // CHECK: instance p2 @PassThrough
     // CHECK-NEXT: instance s @SideEffect2