Skip to content

Commit

Permalink
Fix some issues in the LayerBlockOp verifier
Browse files Browse the repository at this point in the history 
When checking the ops under a layerblock:

- When checking if a child op refers to a value defined outside the layerblock,
  we need to check if the layerblock is an ancestor of the value's definition,
  not the more strict "direct parent of definition" check. This check better
  handles blocks (such as when ops) nested under a layerblock.
- When checking if an child op's operand is valid, we shouldn't early-return,
  which would disables all remaining verification for the op.
- Explicitly allow ref.define op to define destinations outside the layerblock
  op. It is the only connect-like op that is allowed to do so.
- Use WalkResult::interrupt() to signal failure.
  • Loading branch information
@rwy7
rwy7 committed Feb 6, 2024
1 parent 6692e54 commit 6ec11c0
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 40 deletions.
93 changes: 53 additions & 40 deletions lib/Dialect/FIRRTL/FIRRTLOps.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5919,47 +5919,60 @@ LogicalResult LayerBlockOp::verify() {
}

// Verify the body of the region.
Block *body = getBody(0);
bool failed = false;
body->walk<mlir::WalkOrder::PreOrder>([&](Operation *op) {
// Skip nested layer blocks. Those will be verified separately.
if (isa<LayerBlockOp>(op))
return WalkResult::skip();
// Check all the operands of each op to make sure that only legal things are
// captured.
for (auto operand : op->getOperands()) {
// Any value captured from the current layer block is fine.
if (operand.getParentBlock() == body)
continue;
// Capture of a non-base type, e.g., reference, is allowed.
FIRRTLBaseType baseType = dyn_cast<FIRRTLBaseType>(operand.getType());
if (!baseType)
return WalkResult::advance();
// Capturing a non-passive type is illegal.
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";
failed = true;
return WalkResult::advance();
}
}
// Ensure that the layer block does not drive any sinks.
if (auto connect = dyn_cast<FConnectLike>(op)) {
auto dest = getFieldRefFromValue(connect.getDest()).getValue();
if (dest.getParentBlock() == body)
auto result = getBody(0)->walk<mlir::WalkOrder::PreOrder>(
[&](Operation *op) -> WalkResult {
// Skip nested layer blocks. Those will be verified separately.
if (isa<LayerBlockOp>(op))
return WalkResult::skip();

// Check all the operands of each op to make sure that only legal things
// are captured.
for (auto operand : op->getOperands()) {
// Any value captured from the current layer block is fine.
if (auto *definingOp = operand.getDefiningOp())
if (getOperation()->isAncestor(definingOp))
continue;

// Capture of a non-base type, e.g., reference, is allowed.
FIRRTLBaseType type = dyn_cast<FIRRTLBaseType>(operand.getType());
if (!type)
continue;

// Capturing a non-passive type is illegal.
if (!type.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.
if (auto connect = dyn_cast<FConnectLike>(op)) {
// ref.define is allowed to drive probes outside the layerblock.
if (isa<RefDefineOp>(connect))
return WalkResult::advance();

// We can drive any destination inside the current layerblock.
auto dest = getFieldRefFromValue(connect.getDest()).getValue();
if (auto *destOp = dest.getDefiningOp())
if (getOperation()->isAncestor(destOp))
return WalkResult::advance();

auto diag =
connect.emitOpError()
<< "connects to a destination which is defined outside its "
"enclosing layer block";
diag.attachNote(getLoc()) << "enclosing layer block is defined here";
diag.attachNote(dest.getLoc()) << "destination is defined here";
return WalkResult::interrupt();
}

return WalkResult::advance();
auto diag = connect.emitOpError()
<< "connects to a destination which is defined outside its "
"enclosing layer block";
diag.attachNote(getLoc()) << "enclosing layer block is defined here";
diag.attachNote(dest.getLoc()) << "destination is defined here";
failed = true;
}
return WalkResult::advance();
});
if (failed)
});

if (result.wasInterrupted())
return failure();

return success();
Expand Down
63 changes: 63 additions & 0 deletions test/Dialect/FIRRTL/layers.mlir
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// RUN: circt-opt %s | FileCheck %s

firrtl.circuit "Test" {
firrtl.module @Test() {}

// CHECK-LABEL: firrtl.layer @A bind
firrtl.layer @A bind {}

// CHECK-LABEL: firrtl.module @WhenUnderLayer(in %test: !firrtl.uint<1>) {
// CHECK-NEXT: %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
// CHECK-NEXT: firrtl.layerblock @A {
// CHECK-NEXT: %w = firrtl.wire : !firrtl.uint<1>
// CHECK-NEXT: firrtl.when %test : !firrtl.uint<1> {
// CHECK-NEXT: firrtl.strictconnect %w, %c0_ui1 : !firrtl.uint<1>
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK-NEXT: }
firrtl.module @WhenUnderLayer(in %test: !firrtl.uint<1>) {
%c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
firrtl.layerblock @A {
%w = firrtl.wire : !firrtl.uint<1>
firrtl.when %test : !firrtl.uint<1> {
firrtl.strictconnect %w, %c0_ui1 : !firrtl.uint<1>
}
}
}

// CHECK-LABEL: firrtl.module @ProbeEscapeLayer(out %p: !firrtl.probe<uint<1>, @A>) {
// CHECK-NEXT: %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
// CHECK-NEXT: firrtl.layerblock @A {
// CHECK-NEXT: %0 = firrtl.ref.send %c0_ui1 : !firrtl.uint<1>
// CHECK-NEXT: %1 = firrtl.ref.cast %0 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
// CHECK-NEXT: firrtl.ref.define %p, %1 : !firrtl.probe<uint<1>, @A>
// CHECK-NEXT: }
// CHECK-NEXT: }
firrtl.module @ProbeEscapeLayer(out %p: !firrtl.probe<uint<1>, @A>) {
%c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
firrtl.layerblock @A {
%0 = firrtl.ref.send %c0_ui1 : !firrtl.uint<1>
%1 = firrtl.ref.cast %0 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
firrtl.ref.define %p, %1 : !firrtl.probe<uint<1>, @A>
}
}

// CHECK-LABEL: firrtl.module @Layers4(out %o: !firrtl.openbundle<p: probe<uint<1>, @A>>) {
// CHECK-NEXT: firrtl.layerblock @A {
// CHECK-NEXT: %0 = firrtl.opensubfield %o[p] : !firrtl.openbundle<p: probe<uint<1>, @A>>
// CHECK-NEXT: %c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
// CHECK-NEXT: %1 = firrtl.ref.send %c0_ui1 : !firrtl.uint<1>
// CHECK-NEXT: %2 = firrtl.ref.cast %1 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
// CHECK-NEXT: firrtl.ref.define %0, %2 : !firrtl.probe<uint<1>, @A>
// CHECK-NEXT: }
// CHECK-NEXT: }
firrtl.module @Layers4(out %o: !firrtl.openbundle<p: probe<uint<1>, @A>>) {
firrtl.layerblock @A {
%0 = firrtl.opensubfield %o[p] : !firrtl.openbundle<p: probe<uint<1>, @A>>
%c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
%1 = firrtl.ref.send %c0_ui1 : !firrtl.uint<1>
%2 = firrtl.ref.cast %1 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
firrtl.ref.define %0, %2 : !firrtl.probe<uint<1>, @A>
}
}
}

0 comments on commit 6ec11c0

Please sign in to comment.