Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow propassign under layerblocks #6656

Merged
merged 1 commit into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/circt/Dialect/FIRRTL/FIRRTLStatements.td
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
24 changes: 15 additions & 9 deletions lib/Dialect/FIRRTL/FIRRTLOps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6087,20 +6087,26 @@ LogicalResult LayerBlockOp::verify() {
if (getOperation()->isAncestor(definingOp))
continue;

// Capture of a non-base type, e.g., reference, is allowed.
rwy7 marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
38 changes: 38 additions & 0 deletions test/Dialect/FIRRTL/layers-errors.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
14 changes: 14 additions & 0 deletions test/Dialect/FIRRTL/layers.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
Loading