From 73fa06a0e5d8c6a6803f2722f356cc926daf9238 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Thu, 4 Jan 2024 18:01:40 -0500 Subject: [PATCH 1/4] [FIRRTL] Parse Layer-Associated Probes Add support to the FIRRTL parse to handle probes associated with layers. Signed-off-by: Schuyler Eldridge --- lib/Dialect/FIRRTL/Import/FIRParser.cpp | 26 ++++++++++++++++++++++--- test/Dialect/FIRRTL/parse-basic.fir | 6 +++++- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index 4d2a7f8e9d14..3488139864db 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -935,10 +935,20 @@ ParseResult FIRParser::parseType(FIRRTLType &result, const Twine &message) { auto loc = getToken().getLoc(); consumeToken(); FIRRTLType type; + SmallVector layers; 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(); + while (getToken().getKind() == FIRToken::identifier) { + StringRef layer; + if (parseId(layer, "expected layer name")) + return failure(); + layers.push_back(layer); + if (getToken().getKind() == FIRToken::period) + consumeToken(); + } + if (parseToken(FIRToken::greater, "expected '>' in reference type")) return failure(); bool forceable = kind == FIRToken::kw_RWProbe; @@ -953,7 +963,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; + 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)); + } + + result = RefType::get(innerType, forceable, layer); break; } diff --git a/test/Dialect/FIRRTL/parse-basic.fir b/test/Dialect/FIRRTL/parse-basic.fir index fd2aaa44fa98..284b721bd222 100644 --- a/test/Dialect/FIRRTL/parse-basic.fir +++ b/test/Dialect/FIRRTL/parse-basic.fir @@ -1671,9 +1671,13 @@ circuit Layers: ; CHECK-NEXT: } ; CHECK-NEXT: } - ; CHECK: firrtl.module @Layers + ; CHECK: firrtl.module @Layers + ; CHECK-SAME: out %b: !firrtl.probe, @A> + ; CHECK-SAME: out %c: !firrtl.rwprobe, @A::@B> module Layers: input a: UInt<1> + output b: Probe, A> + output c: RWProbe, A.B> layerblock A: node A_a = a From e93208f04e2737dca1b14c56d6304d70a2bcd6e6 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Thu, 4 Jan 2024 18:18:46 -0500 Subject: [PATCH 2/4] [FIRRTL] Emit Layer-associated Probes Add emitter support for layer-associated probes. Signed-off-by: Schuyler Eldridge --- lib/Dialect/FIRRTL/Export/FIREmitter.cpp | 8 ++++++++ test/Dialect/FIRRTL/emit-basic.mlir | 9 +++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/Dialect/FIRRTL/Export/FIREmitter.cpp b/lib/Dialect/FIRRTL/Export/FIREmitter.cpp index 82b00f6f9f18..6fb7dbed770d 100644 --- a/lib/Dialect/FIRRTL/Export/FIREmitter.cpp +++ b/lib/Dialect/FIRRTL/Export/FIREmitter.cpp @@ -1378,6 +1378,14 @@ void Emitter::emitType(Type type, bool includeConst) { ps << "RW"; ps << "Probe<"; emitType(type.getType()); + if (auto layer = type.getLayer()) { + ps << "," << PP::nbsp; + ps.addAsString(layer.getRootReference().getValue()); + for (auto nested : layer.getNestedReferences()) { + ps << "."; + ps.addAsString(nested.getValue()); + } + } ps << ">"; }) .Case([&](AnyRefType type) { ps << "AnyRef"; }) diff --git a/test/Dialect/FIRRTL/emit-basic.mlir b/test/Dialect/FIRRTL/emit-basic.mlir index 5989c26fdc9e..52311f4dc451 100644 --- a/test/Dialect/FIRRTL/emit-basic.mlir +++ b/test/Dialect/FIRRTL/emit-basic.mlir @@ -704,13 +704,18 @@ firrtl.circuit "Foo" { } } // CHECK: module ModuleWithGroups : - // CHECK-NEXT: layerblock GroupA : + // CHECK-NEXT: output a : Probe, GroupA> + // CHECK-NEXT: output b : RWProbe, 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, @GroupA>, + out %b: !firrtl.rwprobe, @GroupA::@GroupB> + ) { firrtl.layerblock @GroupA { firrtl.layerblock @GroupA::@GroupB { firrtl.layerblock @GroupA::@GroupB::@GroupC { From adab52970d1077da0fc27ed381233deb4e31c635 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 12 Jan 2024 18:35:03 -0500 Subject: [PATCH 3/4] fixup! [FIRRTL] Parse Layer-Associated Probes --- lib/Dialect/FIRRTL/Import/FIRParser.cpp | 8 +++++--- test/Dialect/FIRRTL/parse-errors.fir | 8 ++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index 3488139864db..75be1cb3205a 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -942,14 +942,16 @@ ParseResult FIRParser::parseType(FIRRTLType &result, const Twine &message) { return failure(); while (getToken().getKind() == FIRToken::identifier) { StringRef layer; - if (parseId(layer, "expected layer name")) - return failure(); + loc = getToken().getLoc(); + (void)parseId(layer, "expected layer name"); layers.push_back(layer); if (getToken().getKind() == FIRToken::period) consumeToken(); } - if (parseToken(FIRToken::greater, "expected '>' in reference type")) + if (!consumeIf(FIRToken::greater)) { + emitError(loc, "expected '>' to end reference type"); return failure(); + } bool forceable = kind == FIRToken::kw_RWProbe; diff --git a/test/Dialect/FIRRTL/parse-errors.fir b/test/Dialect/FIRRTL/parse-errors.fir index 3e7e323fd2ca..a24db3222594 100644 --- a/test/Dialect/FIRRTL/parse-errors.fir +++ b/test/Dialect/FIRRTL/parse-errors.fir @@ -1253,3 +1253,11 @@ circuit Foo: option Platform: FPGA FPGA + +;// ----- +FIRRTL version 4.0.0 +circuit Foo: + layer A bind: + module Foo: + ; expected-error @below {{expected '>' to end reference type}} + output a: Probe From 41ae2b579cdbb1fabefaeab4297fad8e61baefec Mon Sep 17 00:00:00 2001 From: Robert Young Date: Wed, 24 Jan 2024 10:41:11 -0500 Subject: [PATCH 4/4] Add a fir version check for colored probes --- lib/Dialect/FIRRTL/Export/FIREmitter.cpp | 26 ++++++++---- lib/Dialect/FIRRTL/Import/FIRParser.cpp | 31 ++++++++------ test/Dialect/FIRRTL/emit-basic.mlir | 27 ++++++++++++ test/Dialect/FIRRTL/parse-errors.fir | 53 ++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 20 deletions(-) diff --git a/lib/Dialect/FIRRTL/Export/FIREmitter.cpp b/lib/Dialect/FIRRTL/Export/FIREmitter.cpp index 6fb7dbed770d..53ed33c572b5 100644 --- a/lib/Dialect/FIRRTL/Export/FIREmitter.cpp +++ b/lib/Dialect/FIRRTL/Export/FIREmitter.cpp @@ -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); + 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) { @@ -1377,16 +1389,16 @@ void Emitter::emitType(Type type, bool includeConst) { if (type.getForceable()) ps << "RW"; ps << "Probe<"; + ps.cbox(2, IndentStyle::Block); + ps.zerobreak(); emitType(type.getType()); if (auto layer = type.getLayer()) { - ps << "," << PP::nbsp; - ps.addAsString(layer.getRootReference().getValue()); - for (auto nested : layer.getNestedReferences()) { - ps << "."; - ps.addAsString(nested.getValue()); - } + ps << ","; + ps.space(); + emitSymbol(type.getLayer()); } - ps << ">"; + ps << BreakToken(0, -2) << ">"; + ps.end(); }) .Case([&](AnyRefType type) { ps << "AnyRef"; }) .Case([&](StringType type) { ps << "String"; }) diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index 75be1cb3205a..d44b96c40f53 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -934,25 +934,30 @@ ParseResult FIRParser::parseType(FIRRTLType &result, const Twine &message) { auto kind = getToken().getKind(); auto loc = getToken().getLoc(); consumeToken(); - FIRRTLType type; - SmallVector layers; + // Inner Type + FIRRTLType type; if (parseToken(FIRToken::less, "expected '<' in reference type") || parseType(type, "expected probe data type")) return failure(); - while (getToken().getKind() == FIRToken::identifier) { - StringRef layer; - loc = getToken().getLoc(); - (void)parseId(layer, "expected layer name"); - layers.push_back(layer); - if (getToken().getKind() == FIRToken::period) - consumeToken(); - } - if (!consumeIf(FIRToken::greater)) { - emitError(loc, "expected '>' to end reference type"); - return failure(); + + // Probe Color + SmallVector 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")) + 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(type); diff --git a/test/Dialect/FIRRTL/emit-basic.mlir b/test/Dialect/FIRRTL/emit-basic.mlir index 52311f4dc451..c77e52c99786 100644 --- a/test/Dialect/FIRRTL/emit-basic.mlir +++ b/test/Dialect/FIRRTL/emit-basic.mlir @@ -730,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 + // CHECK-NEXT: > + firrtl.module @ModuleWithLongProbeColor( + out %o: !firrtl.probe, @Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890::@Group1234567890> + ) {} + // CHECK: module RWProbe : // CHECK-NEXT: input in : { a : UInt<1> }[2] // CHECK-NEXT: output p : RWProbe> diff --git a/test/Dialect/FIRRTL/parse-errors.fir b/test/Dialect/FIRRTL/parse-errors.fir index a24db3222594..a9b7e103cccb 100644 --- a/test/Dialect/FIRRTL/parse-errors.fir +++ b/test/Dialect/FIRRTL/parse-errors.fir @@ -1254,6 +1254,44 @@ circuit Foo: 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, 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: @@ -1261,3 +1299,18 @@ circuit Foo: module Foo: ; expected-error @below {{expected '>' to end reference 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 A + +;// ----- +FIRRTL version 4.0.0 +circuit Foo: + layer A bind: + module Foo: + ; expected-error @below {{expected layer name}} + output a: Probe A.>