[FIRRTL] Mark don't touch module ports as overdefined (#1796)

There was nothing forcing these ports to be overdefined before, so constants could leak through.
The undefined flag gets propagated to the instance port moments later.

One could remove the don't touch check in visitConnect now, but it's not clear if checking the attributes is cheaper than merging some lattice values.

lib/Dialect/FIRRTL/Transforms/IMConstProp.cpp

@@ -474,6 +474,11 @@ void IMConstPropPass::markInstanceOp(InstanceOp instance) {
     // Otherwise we have a result from the instance.  We need to forward results
     // from the body to this instance result's SSA value, so remember it.
     BlockArgument modulePortVal = fModule.getPortArgument(resultNo);
+
+    // Mark don't touch results as overdefined
+    if (AnnotationSet::get(modulePortVal).hasDontTouch())
+      markOverdefined(modulePortVal);
+
     resultPortToInstanceResultMapping[modulePortVal].push_back(instancePortVal);
 
     // If there is already a value known for modulePortVal make sure to forward

test/Dialect/FIRRTL/imconstprop.mlir

@@ -169,6 +169,7 @@ firrtl.circuit "Issue1188"  {
 }
 
 // -----
+
 // DontTouch annotation should block constant propagation.
 firrtl.circuit "testDontTouch"  {
   // CHECK-LABEL: firrtl.module @blockProp
@@ -223,9 +224,10 @@ firrtl.circuit "testDontTouch"  {
 
 }
 
+// -----
+
 firrtl.circuit "OutPortTop" {
-    firrtl.module @OutPortChild1(out %out: !firrtl.uint<1>) attributes {
-      portAnnotations =[[{class = "firrtl.transforms.DontTouchAnnotation"}]]} {
+    firrtl.module @OutPortChild1(out %out: !firrtl.uint<1> {firrtl.annotations = [{class = "firrtl.transforms.DontTouchAnnotation"}]}) {
       %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
       firrtl.connect %out, %c0_ui1 : !firrtl.uint<1>, !firrtl.uint<1>
     }
@@ -234,17 +236,23 @@ firrtl.circuit "OutPortTop" {
       firrtl.connect %out, %c0_ui1 : !firrtl.uint<1>, !firrtl.uint<1>
     }
   // CHECK-LABEL: firrtl.module @OutPortTop
-    firrtl.module @OutPortTop(in %x: !firrtl.uint<1>, out %z: !firrtl.uint<1>) {
+    firrtl.module @OutPortTop(in %x: !firrtl.uint<1>, out %zc: !firrtl.uint<1>, out %zn: !firrtl.uint<1>) {
       %c_out = firrtl.instance @OutPortChild1  {name = "c"} : !firrtl.uint<1>
       %c_out_0 = firrtl.instance @OutPortChild2  {name = "c"} : !firrtl.uint<1>
       // CHECK: %0 = firrtl.and %x, %c_out
       %0 = firrtl.and %x, %c_out : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1>
-      // CHECK: %1 = firrtl.and %0, %c0_ui1
-      %1 = firrtl.and %0, %c_out_0 : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1>
-      firrtl.connect %z, %1 : !firrtl.uint<1>, !firrtl.uint<1>
+      // CHECK: %c0_ui1_1 = firrtl.constant 0 
+      %1 = firrtl.and %x, %c_out_0 : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<1>
+      // CHECK: firrtl.connect %zn, %0
+      firrtl.connect %zn, %0 : !firrtl.uint<1>, !firrtl.uint<1>
+      // CHECK: firrtl.connect %zc, %c0_ui1_1
+      firrtl.connect %zc, %1 : !firrtl.uint<1>, !firrtl.uint<1>
     }
 }
 
+
+// -----
+
 firrtl.circuit "InputPortTop"   {
   // CHECK-LABEL: firrtl.module @InputPortChild2
   firrtl.module @InputPortChild2(in %in0: !firrtl.uint<1>, in %in1: !firrtl.uint<1>, out %out: !firrtl.uint<1>) {
@@ -272,6 +280,9 @@ firrtl.circuit "InputPortTop"   {
     firrtl.connect %c2_in1, %c1_ui1 : !firrtl.uint<1>, !firrtl.uint<1>
   }
 }
+
+// -----
+
 firrtl.circuit "InstanceOut"   {
   firrtl.extmodule @Ext(in %a: !firrtl.uint<1>)
 
@@ -286,6 +297,9 @@ firrtl.circuit "InstanceOut"   {
     firrtl.connect %b, %w : !firrtl.uint<1>, !firrtl.uint<1>
   }
 }
+
+// -----
+
 firrtl.circuit "InstanceOut2"   {
   firrtl.module @Ext(in %a: !firrtl.uint<1>) {
   }
@@ -301,6 +315,9 @@ firrtl.circuit "InstanceOut2"   {
     firrtl.connect %b, %w : !firrtl.uint<1>, !firrtl.uint<1>
   }
 }
+
+// -----
+
 firrtl.circuit "invalidReg1"   {
   // CHECK_LABEL: @invalidReg1
   firrtl.module @invalidReg1(in %clock: !firrtl.clock, out %a: !firrtl.uint<1>) {
@@ -313,6 +330,9 @@ firrtl.circuit "invalidReg1"   {
       firrtl.connect %a, %foobar : !firrtl.uint<1>, !firrtl.uint<1>
   }
 }
+
+// -----
+
 firrtl.circuit "invalidReg2"   {
   // CHECK_LABEL: @invalidReg2
   firrtl.module @invalidReg2(in %clock: !firrtl.clock, out %a: !firrtl.uint<1>) {
@@ -324,6 +344,8 @@ firrtl.circuit "invalidReg2"   {
   }
 }
 
+// -----
+
 // This test is checking the behavior of a RegOp, "r", and a RegResetOp, "s",
 // that are combinationally connected to themselves through simple and weird
 // formulations.  In all cases it should NOT be optimized away.  For more discussion, see:
@@ -413,6 +435,8 @@ firrtl.circuit "Oscillators"   {
   }
 }
 
+// -----
+
 // This test checks that an output port sink, used as a RHS of a connect, is not
 // optimized away.  This is similar to the oscillator tests above, but more
 // reduced. See:
@@ -438,6 +462,7 @@ firrtl.circuit "rhs_sink_output_used_as_wire" {
   }
 }
 
+// -----
 
 firrtl.circuit "constRegReset" {
 // CHECK-LABEL: firrtl.module @constRegReset
@@ -452,6 +477,8 @@ firrtl.module @constRegReset(in %clock: !firrtl.clock, in %reset: !firrtl.uint<1
 }
 }
 
+// -----
+
 firrtl.circuit "constRegReset2" {
 // CHECK-LABEL: firrtl.module @constRegReset2
 firrtl.module @constRegReset2(in %clock: !firrtl.clock, in %reset: !firrtl.uint<1>, in %cond: !firrtl.uint<1>, out %z: !firrtl.uint<8>) {
@@ -466,6 +493,8 @@ firrtl.module @constRegReset2(in %clock: !firrtl.clock, in %reset: !firrtl.uint<
 }
 }
 
+// -----
+
 firrtl.circuit "regMuxTree"   {
   // CHECK-LABEL: firrtl.module @regMuxTree
   firrtl.module @regMuxTree(in %clock: !firrtl.clock, in %reset: !firrtl.uint<1>, in %cmd: !firrtl.uint<3>, out %z: !firrtl.uint<8>) {
@@ -493,3 +522,23 @@ firrtl.circuit "regMuxTree"   {
     // CHECK:  firrtl.connect %z, %[[c7_ui8]] : !firrtl.uint<8>, !firrtl.uint<8>
   }
 }
+
+// -----
+
+// issue 1793
+// Ensure don't touch on output port is seen by instances
+firrtl.circuit "dntOutput" {
+  // CHECK-LABEL: firrtl.module @dntOutput
+  // CHECK: %0 = firrtl.mux(%c, %int_b, %c2_ui3)
+  // CHECK-NEXT: firrtl.connect %b, %0
+  firrtl.module @dntOutput(out %b : !firrtl.uint<3>, in %c : !firrtl.uint<1>) {
+    %const = firrtl.constant 2 : !firrtl.uint<3>
+    %int_b = firrtl.instance @foo  {name = "int"} : !firrtl.uint<3>
+    %m = firrtl.mux(%c, %int_b, %const) : (!firrtl.uint<1>, !firrtl.uint<3>, !firrtl.uint<3>) -> !firrtl.uint<3>
+    firrtl.connect %b, %m : !firrtl.uint<3>, !firrtl.uint<3>
+  }
+  firrtl.module @foo(out %b: !firrtl.uint<3> {firrtl.annotations = [{class = "firrtl.transforms.DontTouchAnnotation"}]}) {
+    %const = firrtl.constant 1 : !firrtl.uint<3>
+    firrtl.connect %b, %const : !firrtl.uint<3>, !firrtl.uint<3>
+  }
+}