From cb5d02799c94d27f106df3447a3ba65c89407071 Mon Sep 17 00:00:00 2001
From: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Date: Sat, 9 Apr 2022 17:54:20 -0400
Subject: [PATCH] [FIRRTL] Squash Invalids in RemoveInvalid

Extend the RemoveInvalid pass (formerly RemoveResets) to convert all
invalid values to zero.  This is done to apply the "invalid is zero"
interpretation after all other invalid interpretations have been
resolved.  This replaces logic where this interpretation was
incompletely applied during canonicalization after RemoveResets.

Fixes #2782.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
---
 .../FIRRTL/Transforms/RemoveInvalid.cpp       | 40 ++++++++++++++++++-
 .../SFCTests/invalid-interpretations.fir      | 31 ++++++++++++++
 test/firtool/firtool.fir                      |  2 +-
 3 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/lib/Dialect/FIRRTL/Transforms/RemoveInvalid.cpp b/lib/Dialect/FIRRTL/Transforms/RemoveInvalid.cpp
index 4a9fcd7479..de5525d75d 100644
--- a/lib/Dialect/FIRRTL/Transforms/RemoveInvalid.cpp
+++ b/lib/Dialect/FIRRTL/Transforms/RemoveInvalid.cpp
@@ -15,6 +15,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "PassDetails.h"
+#include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
 #include "circt/Dialect/FIRRTL/Passes.h"
 #include "mlir/IR/ImplicitLocOpBuilder.h"
 #include "llvm/ADT/TypeSwitch.h"
@@ -100,8 +101,16 @@ void RemoveInvalidPass::runOnOperation() {
                    << "Module: '" << getOperation().getName() << "'\n";);
 
   bool madeModifications = false;
-  for (auto reg : llvm::make_early_inc_range(
-           getOperation().getBody()->getOps<RegResetOp>())) {
+  SmallVector<InvalidValueOp> invalidOps;
+  for (auto &op : llvm::make_early_inc_range(getOperation().getOps())) {
+    // Populate invalidOps for later handling.
+    if (auto inv = dyn_cast<InvalidValueOp>(op)) {
+      invalidOps.push_back(inv);
+      continue;
+    }
+    auto reg = dyn_cast<RegResetOp>(op);
+    if (!reg)
+      continue;
 
     // If the `RegResetOp` has an invalidated initialization, then replace it
     // with a `RegOp`.
@@ -118,6 +127,33 @@ void RemoveInvalidPass::runOnOperation() {
     }
   }
 
+  // Convert all invalid values to zero.
+  for (auto inv : invalidOps) {
+    // Skip invalids which have no uses.
+    if (inv->getUses().empty())
+      continue;
+    ImplicitLocOpBuilder builder(inv.getLoc(), inv);
+    TypeSwitch<FIRRTLType>(inv.getType())
+        .Case<ClockType, AsyncResetType, ResetType>([&](auto type) {
+          auto zero = builder.create<SpecialConstantOp>(
+              type, builder.getBoolAttr(false));
+          inv->replaceAllUsesWith(zero);
+          inv->erase();
+          madeModifications = true;
+        })
+        .Case<IntType>([&](IntType type) {
+          auto zero = builder.create<ConstantOp>(type, getIntZerosAttr(type));
+          inv->replaceAllUsesWith(zero);
+          inv->erase();
+          madeModifications = true;
+        })
+        .Default([&](auto) {
+          inv->emitOpError()
+              << "has a type which is not a ground type (this is an "
+                 "unimplemented feature of RemoveInvalid!)";
+        });
+  }
+
   if (!madeModifications)
     return markAllAnalysesPreserved();
 }
diff --git a/test/Dialect/FIRRTL/SFCTests/invalid-interpretations.fir b/test/Dialect/FIRRTL/SFCTests/invalid-interpretations.fir
index 893768cae1..75e91121b4 100644
--- a/test/Dialect/FIRRTL/SFCTests/invalid-interpretations.fir
+++ b/test/Dialect/FIRRTL/SFCTests/invalid-interpretations.fir
@@ -42,3 +42,34 @@ circuit InvalidInterpretations:
     ; Interpretation 4: Invalid is zero otherwise.
     ; CHECK:       assign out_mux = cond ? a : 8'h0;
     ; CHECK-NEXT:  assign out_add = {1'h0, a};
+
+; // -----
+
+; This is checking that an invalid value in another module is not propagated,
+; but is interpreted as "zero".  The end result of this is that the main module
+; should have a register that will be reset to a non-zero value, but is
+; constantly driven to zero by default.
+;
+; See: https://github.com/llvm/circt/issues/2782
+
+; CHECK-LABEL: module InvalidInOtherModule
+circuit InvalidInOtherModule :
+  module InvalidInOtherModule :
+    input clock: Clock
+    input reset: UInt<1>
+    output b: SInt<8>
+
+    inst other of OtherModule
+
+    ; CHECK:      always @(posedge clock)
+    ; CHECK-NEXT:   if (reset)
+    ; CHECK-NEXT:     r <= 8'h4;
+    ; CHECK-NEXT:   else
+    ; CHECK-NEXT:     r <= 8'h0;
+    reg r : SInt<8>, clock with: (reset => (reset, SInt<8>(4)))
+    r <= other.b
+    b <= r
+
+  module OtherModule :
+    output b: SInt<8>
+    b is invalid
diff --git a/test/firtool/firtool.fir b/test/firtool/firtool.fir
index 66720ab066..f5d01890c8 100644
--- a/test/firtool/firtool.fir
+++ b/test/firtool/firtool.fir
@@ -105,7 +105,7 @@ circuit test_mod : %[[{"a": "a"}]]
 ; MLIR-NEXT:    firrtl.strictconnect %multibitMux_a_1, %vec_1 : !firrtl.uint<1>
 ; MLIR-NEXT:    firrtl.strictconnect %multibitMux_a_2, %vec_2 : !firrtl.uint<1>
 ; MLIR-NEXT:    firrtl.strictconnect %multibitMux_sel, %b : !firrtl.uint<2>
-; MLIR-NEXT:    %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1>
+; MLIR-NEXT:    %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
 ; MLIR-NEXT:    firrtl.instance unusedPortsMod @UnusedPortsMod()
 ; MLIR-NEXT:  }