From a8cd7c842b44de4041352a549fbb617029342246 Mon Sep 17 00:00:00 2001
From: Robert Young <rwy0717@gmail.com>
Date: Tue, 6 Feb 2024 19:33:46 -0500
Subject: [PATCH] Allow propassign under layerblocks

Also, ban capturing outer properties inside a layerblock.
---
 .../circt/Dialect/FIRRTL/FIRRTLStatements.td  |  4 +-
 lib/Dialect/FIRRTL/FIRRTLOps.cpp              | 24 +++++++-----
 test/Dialect/FIRRTL/layers-errors.mlir        | 38 +++++++++++++++++++
 test/Dialect/FIRRTL/layers.mlir               | 14 +++++++
 4 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/include/circt/Dialect/FIRRTL/FIRRTLStatements.td b/include/circt/Dialect/FIRRTL/FIRRTLStatements.td
index 31d3b11c6c3..eaad44a8813 100644
--- a/include/circt/Dialect/FIRRTL/FIRRTLStatements.td
+++ b/include/circt/Dialect/FIRRTL/FIRRTLStatements.td
@@ -278,8 +278,8 @@ def MatchOp : FIRRTLOp<"match", [SingleBlock, NoTerminator,
   }];
 }
 
-def PropAssignOp : FIRRTLOp<"propassign",
-  [FConnectLike, SameTypeOperands, ParentOneOf<["FModuleOp", "ClassOp"]>]> {
+def PropAssignOp : FIRRTLOp<"propassign", [FConnectLike, SameTypeOperands,
+    ParentOneOf<["FModuleOp", "ClassOp", "LayerBlockOp"]>]> {
   let summary = "Assign to a sink property value.";
   let description = [{
     Assign an output property value. The types must match exactly.
diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp
index f7810f3160b..7834dd6046c 100644
--- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp
+++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp
@@ -6087,20 +6087,26 @@ LogicalResult LayerBlockOp::verify() {
             if (getOperation()->isAncestor(definingOp))
               continue;
 
-          // Capture of a non-base type, e.g., reference, is allowed.
-          FIRRTLBaseType type =
-              type_dyn_cast<FIRRTLBaseType>(operand.getType());
-          if (!type)
-            continue;
+          auto type = operand.getType();
 
-          // Capturing a non-passive type is illegal.
-          if (!type.isPassive()) {
-            auto diag = emitOpError()
-                        << "captures an operand which is not a passive type";
+          // Capture of a non-base type, e.g., reference, is allowed.
+          if (isa<PropertyType>(type)) {
+            auto diag = emitOpError() << "captures a property operand";
             diag.attachNote(operand.getLoc()) << "operand is defined here";
             diag.attachNote(op->getLoc()) << "operand is used here";
             return WalkResult::interrupt();
           }
+
+          // Capturing a non-passive type is illegal.
+          if (auto baseType = type_dyn_cast<FIRRTLBaseType>(type)) {
+            if (!baseType.isPassive()) {
+              auto diag = emitOpError()
+                          << "captures an operand which is not a passive type";
+              diag.attachNote(operand.getLoc()) << "operand is defined here";
+              diag.attachNote(op->getLoc()) << "operand is used here";
+              return WalkResult::interrupt();
+            }
+          }
         }
 
         // Ensure that the layer block does not drive any sinks outside.
diff --git a/test/Dialect/FIRRTL/layers-errors.mlir b/test/Dialect/FIRRTL/layers-errors.mlir
index 596cf242d8d..c2ce129482e 100644
--- a/test/Dialect/FIRRTL/layers-errors.mlir
+++ b/test/Dialect/FIRRTL/layers-errors.mlir
@@ -131,3 +131,41 @@ firrtl.circuit "Top" {
     } ()
   }
 }
+
+// -----
+
+// Capturing an outer property from inside a layerblock is not allowed.
+// Eventually, we may like to relax this, but we need time to convince
+// ourselves whether this is actually safe and can be lowered.
+firrtl.circuit "Top" {
+  firrtl.layer @A bind {}
+  firrtl.extmodule @WithInputProp(in i : !firrtl.string) attributes {layers=[@A]}
+  firrtl.module @Top(out %o : !firrtl.probe<uint<1>>) {
+    // expected-note @below {{operand is defined here}}
+    %str = firrtl.string "whatever"
+    // expected-error @below {{'firrtl.layerblock' op captures a property operand}}
+    firrtl.layerblock @A {
+       %foo_in = firrtl.instance foo @WithInputProp(in i : !firrtl.string)
+       // expected-note @below {{operand is used here}}
+      firrtl.propassign %foo_in, %str : !firrtl.string
+    }
+  }
+}
+
+// -----
+
+// Driving an outer property from inside a layerblock is not allowed.
+firrtl.circuit "Top" {
+  firrtl.layer @A bind {}
+  firrtl.extmodule @WithInputProp(in i : !firrtl.string) attributes {layers=[@A]}
+  firrtl.module @Top(out %o : !firrtl.probe<uint<1>>) {
+    // expected-note @below {{operand is defined here}}
+    %foo_in = firrtl.instance foo @WithInputProp(in i : !firrtl.string)
+    // expected-error @below {{'firrtl.layerblock' op captures a property operand}}
+    firrtl.layerblock @A {
+      %str = firrtl.string "whatever"
+       // expected-note @below {{operand is used here}}
+      firrtl.propassign %foo_in, %str : !firrtl.string
+    }
+  }
+}
diff --git a/test/Dialect/FIRRTL/layers.mlir b/test/Dialect/FIRRTL/layers.mlir
index d6211ffbfd1..f63770c759b 100644
--- a/test/Dialect/FIRRTL/layers.mlir
+++ b/test/Dialect/FIRRTL/layers.mlir
@@ -187,4 +187,18 @@ firrtl.circuit "Test" {
       firrtl.ref.define %0, %2 : !firrtl.probe<uint<1>, @A>
     }
   }
+
+  //===--------------------------------------------------------------------===//
+  // Properties Under Layers
+  //===--------------------------------------------------------------------===//
+
+  firrtl.extmodule @WithInputProp(in i : !firrtl.string)
+
+  firrtl.module @InstWithInputPropUnderLayerBlock() {
+    firrtl.layerblock @A {
+      %foo_in = firrtl.instance foo @WithInputProp(in i : !firrtl.string)
+      %str = firrtl.string "whatever"
+      firrtl.propassign %foo_in, %str : !firrtl.string
+    }
+  }
 }