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.

Sign up for GitHub

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

Jump to bottom

[FIRRTL] Add parser/emitter support for layer-associated probes #6552

Merged
merged 4 commits into from
Jan 29, 2024
Merged

[FIRRTL] Add parser/emitter support for layer-associated probes #6552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73fa06a
[FIRRTL] Parse Layer-Associated Probes
seldridge Jan 4, 2024
e93208f
[FIRRTL] Emit Layer-associated Probes
seldridge Jan 4, 2024
adab529
fixup! [FIRRTL] Parse Layer-Associated Probes
seldridge Jan 12, 2024
41ae2b5
Add a fir version check for colored probes
rwy7 Jan 24, 2024
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
22 changes: 21 additions & 1 deletion lib/Dialect/FIRRTL/Export/FIREmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,18 @@ struct Emitter {
[&](Value v) { emitSubExprIBox2(v); });
}

/// Emit a (potentially nested) symbol reference as `A.B.C`.
void emitSymbol(SymbolRefAttr symbol) {
ps.ibox(2, IndentStyle::Block);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW to help ensure boxes are closed and show their scoping there's ps.scopedBox(PP::ibox2, [&]() { ... }) pattern. This helps in more complicated code esp when there are return paths to "cover" and various nested boxes. But this is fine as-is, just FYI! :)

ps << symbol.getRootReference();
for (auto nested : symbol.getNestedReferences()) {
ps.zerobreak();
ps << ".";
ps << nested.getAttr();
}
ps.end();
}

private:
/// Emit an error and remark that emission failed.
InFlightDiagnostic emitError(Operation *op, const Twine &message) {
Expand Down Expand Up @@ -1377,8 +1389,16 @@ void Emitter::emitType(Type type, bool includeConst) {
if (type.getForceable())
ps << "RW";
ps << "Probe<";
ps.cbox(2, IndentStyle::Block);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

ps << "Probe<" << PP::cbox2 << PP::zerobreak

If you find that neater (and scopedBox(PP::cbox2, ...) if you ilke).

ps.zerobreak();
emitType(type.getType());
ps << ">";
if (auto layer = type.getLayer()) {
ps << ",";
ps.space();
emitSymbol(type.getLayer());
}
ps << BreakToken(0, -2) << ">";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! Sorry for this being a thing and not abstracted away, but GJ 👍 .

ps.end();
})
.Case<AnyRefType>([&](AnyRefType type) { ps << "AnyRef"; })
.Case<StringType>([&](StringType type) { ps << "String"; })
Expand Down
35 changes: 31 additions & 4 deletions lib/Dialect/FIRRTL/Import/FIRParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,13 +934,30 @@ ParseResult FIRParser::parseType(FIRRTLType &result, const Twine &message) {
auto kind = getToken().getKind();
auto loc = getToken().getLoc();
consumeToken();
FIRRTLType type;

// Inner Type
FIRRTLType type;
if (parseToken(FIRToken::less, "expected '<' in reference type") ||
parseType(type, "expected probe data type") ||
parseToken(FIRToken::greater, "expected '>' in reference type"))
parseType(type, "expected probe data type"))
return failure();

// Probe Color
SmallVector<StringRef> layers;
if (getToken().getKind() == FIRToken::identifier) {
if (requireFeature({3, 2, 0}, "colored probes"))
return failure();
do {
StringRef layer;
loc = getToken().getLoc();
if (parseId(layer, "expected layer name"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseId allows more than FIRToken::identifier -- do we want to support these? I think this is important for "literal" identifiers (with ticks) or if the identifier is a keyword re:tokens-- aka a module named when (see FIRTokenKinds.def).

return failure();
layers.push_back(layer);
} while (consumeIf(FIRToken::period));
}

if (!consumeIf(FIRToken::greater))
return emitError(loc, "expected '>' to end reference type");

bool forceable = kind == FIRToken::kw_RWProbe;

auto innerType = type_dyn_cast<FIRRTLBaseType>(type);
Expand All @@ -953,7 +970,17 @@ ParseResult FIRParser::parseType(FIRRTLType &result, const Twine &message) {
if (forceable && innerType.containsConst())
return emitError(loc, "rwprobe cannot contain const");

result = RefType::get(innerType, forceable);
SymbolRefAttr layer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should parser check the validity of the layers?

I haven't gone through the PR's enough yet to understand the verification story/plan/strategy, but at least it'd be good to get a nice parser error if the mentioned identifiers do not exist at all or aren't layers (and perhaps aren't nested layers or whatever?).

I haven't checked whether we insist on deeply parsing layers entirely first or not, similar concerns existed IIRC for type aliases and such (?).

Copy link
Member Author

@seldridge seldridge Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in the camp of no, this shouldn't check explicitly in the parser. My reasoning is that this is duplicating a check that the verifier handles (and presents a good error message for). With the follow-on PR (#6574) the following will error nicely:

FIRRTL version 4.0.0
circuit Foo:

  module Foo:
    output a: Probe<UInt<1>, A>

# ./bin/firtool -parse-only ~/repos/github.com/seldridge/firrtl-snippets/layers/Error.fir
/Users/schuylereldridge/repos/github.com/seldridge/firrtl-snippets/layers/Error.fir:8:12: error: probe port 'a' is associated with layer '@A', but this layer was not defined
    output a: Probe<UInt<1>, A>
           ^

With this PR alone, there is no error.

Are you good with this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you, that sounds fine, thanks! And thanks for care re:diagnostic 👍 .

if (!layers.empty()) {
auto nestedLayers =
llvm::map_range(ArrayRef(layers).drop_front(), [&](StringRef a) {
return FlatSymbolRefAttr::get(getContext(), a);
});
layer = SymbolRefAttr::get(getContext(), layers.front(),
llvm::to_vector(nestedLayers));
seldridge marked this conversation as resolved.
Show resolved Hide resolved
}

result = RefType::get(innerType, forceable, layer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version should gate layer support? Are we still doing that?

Please add error tests.

I'm unsure about starting by supporting parsing something until it's more robustly supported internally -- WDYT?

I don't have good handle on what doesn't work, I think at this point this will parse it but just ignore it everywhere more or less, that about right? That's less unsafe if not a great user experience, and I bet this helps testing/exploring flows from Chisel ASAP too, so if that matches sounds good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can include error tests. I omitted them here as they aren't particularly interesting. E.g., there's really only one error case to check:

  1. Probe<UInt<1>, A

This increases code coverage and is probably good to test.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it looks like this test should have been there before and was missing. 🙃 I'll add it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added in dea1a8e.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering your larger question... I generally structure new features into several distinct PRs:

  1. Add basic dialect support.
  2. Add parser/emitter support.
  3. Add verifier support (possibly as part of 1, but you usually miss stuff in 1 anyway and wind up adding stuff here).
  4. Add pass support, likely broken into multiple PRs adding support in FIRRTL, then in HW/SV. This may also be broken up to handle on a per-pass basis if it's complex.
  5. End-to-end work if not handled in (4).
  6. Chisel API added.

(1) is a prerequisite of (2). Having (2) means you can start writing FIRRTL examples and make sure things work with the spec / iterate on the spec. (3--5) are then necessary to close it out.

As long as you don't add a Chisel API and a new feature is not load bearing (replacing something existing), then there's no need to land this all at once.

I'm going off this playbook here to try and make things easy to review. There is also my strong desire to not duplicate parsing checks in verifiers. If these are duplicated then you wind up having to keep both up to date. I frequently forget about it and things are out of date.

Do you agree with this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version should gate layer support? Are we still doing that?

3.2.0 is the version of the firrtl spec that added colored probes. I will add a feature guard to this PR.

I'm unsure about starting by supporting parsing something until it's more robustly supported internally -- WDYT?

Personally I am OK with delivering changes "front to back", as long as there is a plan to finish up the work, which I think we have :).

I keep thinking, maybe we should have an "enable experimental features" cli option in firtool, that we could use here for a bit of extra sanity while layers are in development (especially considering, the FIRRTL version for this feature is not the unstable/next version)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just own this through to the end, sounds good guys thanks!

break;
}

Expand Down
36 changes: 34 additions & 2 deletions test/Dialect/FIRRTL/emit-basic.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,13 +704,18 @@ firrtl.circuit "Foo" {
}
}
// CHECK: module ModuleWithGroups :
// CHECK-NEXT: layerblock GroupA :
// CHECK-NEXT: output a : Probe<UInt<1>, GroupA>
// CHECK-NEXT: output b : RWProbe<UInt<1>, GroupA.GroupB>
// CHECK: layerblock GroupA :
// CHECK-NEXT: layerblock GroupB :
// CHECK-NEXT: layerblock GroupC :
// CHECK-NEXT: layerblock GroupD :
// CHECK-NEXT: layerblock GroupE :
// CHECK-NEXT: layerblock GroupF :
firrtl.module @ModuleWithGroups() {
firrtl.module @ModuleWithGroups(
out %a: !firrtl.probe<uint<1>, @GroupA>,
out %b: !firrtl.rwprobe<uint<1>, @GroupA::@GroupB>
) {
firrtl.layerblock @GroupA {
firrtl.layerblock @GroupA::@GroupB {
rwy7 marked this conversation as resolved.
Show resolved Hide resolved
firrtl.layerblock @GroupA::@GroupB::@GroupC {
Expand All @@ -725,6 +730,33 @@ firrtl.circuit "Foo" {
}
}

// Test line-breaks for very large layer associations.
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {
firrtl.layer @Group1234567890 bind {}
}
}
}
}
}
}
}

// CHECK: module ModuleWithLongProbeColor
// CHECK-NEXT: output o : Probe<
// CHECK-NEXT: UInt<1>,
// CHECK-NEXT: Group1234567890.Group1234567890.Group1234567890.Group1234567890
// CHECK-NEXT: .Group1234567890.Group1234567890.Group1234567890.Group1234567890
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 😍

// CHECK-NEXT: >
firrtl.module @ModuleWithLongProbeColor(
out %o: !firrtl.probe<uint<1>, @Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890>
) {}

// CHECK: module RWProbe :
// CHECK-NEXT: input in : { a : UInt<1> }[2]
// CHECK-NEXT: output p : RWProbe<UInt<1>>
Expand Down
6 changes: 5 additions & 1 deletion test/Dialect/FIRRTL/parse-basic.fir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,9 +1671,13 @@ circuit Layers:
; CHECK-NEXT: }
; CHECK-NEXT: }

; CHECK: firrtl.module @Layers
; CHECK: firrtl.module @Layers
; CHECK-SAME: out %b: !firrtl.probe<uint<1>, @A>
; CHECK-SAME: out %c: !firrtl.rwprobe<uint<1>, @A::@B>
module Layers:
input a: UInt<1>
output b: Probe<UInt<1>, A>
output c: RWProbe<UInt<1>, A.B>

layerblock A:
node A_a = a
Expand Down
61 changes: 61 additions & 0 deletions test/Dialect/FIRRTL/parse-errors.fir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,3 +1253,64 @@ circuit Foo:
option Platform:
FPGA
FPGA

;// -----
FIRRTL version 3.1.0
circuit Foo:

module Foo:
; expected-error @below {{colored probes are a FIRRTL 3.2.0+ feature, but the specified FIRRTL version was 3.1.0}}
output a: Probe<UInt<1>, A>

;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
output a: Probe<
; expected-error @below {{expected probe data type}}
;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
; expected-error @below {{expected '<' in reference type}}
output a: Probe X

;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
output a: Probe
; expected-error @below {{expected '<' in reference type}}
;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
; expected-error @below {{expected probe data type}}
output a: Probe<>

;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
; expected-error @below {{expected '>' to end reference type}}
output a: Probe<UInt<1>
;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
; expected-error @below {{expected '>' to end reference type}}
output a: Probe<UInt<1> A

;// -----
FIRRTL version 4.0.0
circuit Foo:
layer A bind:
module Foo:
; expected-error @below {{expected layer name}}
output a: Probe<UInt<1> A.>
Loading