From 22b7f10b8d66055512ad2969533e8c4a0fb9d81b Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Wed, 21 Feb 2024 09:14:15 -0600 Subject: [PATCH 1/2] [FIRRTL] Check unknown width and reset rules during parsing 4.0.0. Only check during parsing to stay compatibile with previous behavior. Allow abstract reset on extmodule until spec changes. --- lib/Dialect/FIRRTL/Import/FIRParser.cpp | 21 +++++++++++++++++++++ test/Dialect/FIRRTL/emit-basic.mlir | 12 ++++++------ test/Dialect/FIRRTL/parse-errors.fir | 22 ++++++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index d7ceee302441..93705d0d3381 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -4964,6 +4964,15 @@ ParseResult FIRCircuitParser::parseExtModule(CircuitOp circuit, if (parseParameterList(parameters) || parseRefList(portList, internalPaths)) return failure(); + if (version >= FIRVersion{4, 0, 0}) { + for (auto [pi, loc] : llvm::zip_equal(portList, portLocs)) { + if (auto ftype = dyn_cast(pi.type)) { + if (ftype.hasUninferredWidth()) + return emitError(loc, "extmodule port must have known width"); + } + } + } + auto builder = circuit.getBodyBuilder(); auto isMainModule = (name == circuit.getName()); auto convention = @@ -5039,6 +5048,18 @@ ParseResult FIRCircuitParser::parseModule(CircuitOp circuit, bool isPublic, // The main module is implicitly public. isPublic |= name == circuit.getName(); + if (isPublic && version >= FIRVersion{4, 0, 0}) { + for (auto [pi, loc] : llvm::zip_equal(portList, portLocs)) { + if (auto ftype = dyn_cast(pi.type)) { + if (ftype.hasUninferredWidth()) + return emitError(loc, "public module port must have known width"); + if (ftype.hasUninferredReset()) + return emitError(loc, + "public module port must have concrete reset type"); + } + } + } + ArrayAttr annotations = getConstants().emptyArrayAttr; auto convention = Convention::Internal; if (isPublic && getConstants().options.scalarizePublicModules) diff --git a/test/Dialect/FIRRTL/emit-basic.mlir b/test/Dialect/FIRRTL/emit-basic.mlir index bf0ee4894e87..b2e2354a505c 100644 --- a/test/Dialect/FIRRTL/emit-basic.mlir +++ b/test/Dialect/FIRRTL/emit-basic.mlir @@ -23,7 +23,7 @@ firrtl.circuit "Foo" { firrtl.module private @PrivateModule() {} // CHECK-LABEL: module PortsAndTypes : - firrtl.module @PortsAndTypes( + firrtl.module private @PortsAndTypes( // CHECK-NEXT: input a00 : Clock // CHECK-NEXT: input a01 : Reset // CHECK-NEXT: input a02 : AsyncReset @@ -76,7 +76,7 @@ firrtl.circuit "Foo" { } // CHECK-LABEL: module Statements : - firrtl.module @Statements(in %ui1: !firrtl.uint<1>, in %someAddr: !firrtl.uint<8>, in %someClock: !firrtl.clock, in %someReset: !firrtl.reset, out %someOut: !firrtl.uint<1>, out %ref: !firrtl.probe>) { + firrtl.module private @Statements(in %ui1: !firrtl.uint<1>, in %someAddr: !firrtl.uint<8>, in %someClock: !firrtl.clock, in %someReset: !firrtl.reset, out %someOut: !firrtl.uint<1>, out %ref: !firrtl.probe>) { // CHECK: when ui1 : // CHECK: skip firrtl.when %ui1 : !firrtl.uint<1> { @@ -443,9 +443,9 @@ firrtl.circuit "Foo" { firrtl.ref.define %out_b_0_y_2, %b_0_y_2 : !firrtl.probe> } - firrtl.extmodule @MyParameterizedExtModule(in in: !firrtl.uint, out out: !firrtl.uint<8>) attributes {defname = "name_thing"} + firrtl.extmodule @MyParameterizedExtModule(in in: !firrtl.uint<1>, out out: !firrtl.uint<8>) attributes {defname = "name_thing"} // CHECK-LABEL: extmodule MyParameterizedExtModule : - // CHECK-NEXT: input in : UInt + // CHECK-NEXT: input in : UInt<1> // CHECK-NEXT: output out : UInt<8> // CHECK-NEXT: defname = name_thing // CHECK-NEXT: parameter DEFAULT = 0 @@ -472,7 +472,7 @@ firrtl.circuit "Foo" { // CHECK-NEXT: parameter DEPTH = 32.42 // CHECK-LABEL: module ConstTypes : - firrtl.module @ConstTypes( + firrtl.module private @ConstTypes( // CHECK-NEXT: input a00 : const Clock // CHECK-NEXT: input a01 : const Reset // CHECK-NEXT: input a02 : const AsyncReset @@ -513,7 +513,7 @@ firrtl.circuit "Foo" { in %_0: !firrtl.uint<1> ) attributes {portNames = ["0"]} {} // CHECK-LABEL: module `0Foo` : - firrtl.module @"0Foo"( + firrtl.module private @"0Foo"( // CHECK-NEXT: input `0` : Clock // CHECK-NEXT: input `1` : Reset // CHECK-NEXT: input `2` : AsyncReset diff --git a/test/Dialect/FIRRTL/parse-errors.fir b/test/Dialect/FIRRTL/parse-errors.fir index f2d0097700e6..3ef306bd42b6 100644 --- a/test/Dialect/FIRRTL/parse-errors.fir +++ b/test/Dialect/FIRRTL/parse-errors.fir @@ -1347,3 +1347,25 @@ circuit Foo: module Foo: ; expected-error @below {{expected layer name}} output a: Probe A.> + + +;// ----- +FIRRTL version 4.0.0 +circuit UnknownWidthPublic: + public module UnknownWidthPublic: + ; expected-error @below {{public module port must have known width}} + output a: UInt + +;// ----- +FIRRTL version 4.0.0 +circuit UnknownWidthExt: + extmodule UnknownWidthExt: + ; expected-error @below {{extmodule port must have known width}} + output a: { x: UInt } + +;// ----- +FIRRTL version 4.0.0 +circuit AbstractResetPublic: + module AbstractResetPublic: + ; expected-error @below {{public module port must have concrete reset type}} + input r: Reset From 9579192036442f7505b41c3353333f0684f30d74 Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Fri, 23 Feb 2024 08:11:32 -0600 Subject: [PATCH 2/2] Use type_dyn_cast, whitespace touchup in test. --- lib/Dialect/FIRRTL/Import/FIRParser.cpp | 4 ++-- test/Dialect/FIRRTL/parse-errors.fir | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/Dialect/FIRRTL/Import/FIRParser.cpp b/lib/Dialect/FIRRTL/Import/FIRParser.cpp index 93705d0d3381..43c6b9d2f7b9 100644 --- a/lib/Dialect/FIRRTL/Import/FIRParser.cpp +++ b/lib/Dialect/FIRRTL/Import/FIRParser.cpp @@ -4966,7 +4966,7 @@ ParseResult FIRCircuitParser::parseExtModule(CircuitOp circuit, if (version >= FIRVersion{4, 0, 0}) { for (auto [pi, loc] : llvm::zip_equal(portList, portLocs)) { - if (auto ftype = dyn_cast(pi.type)) { + if (auto ftype = type_dyn_cast(pi.type)) { if (ftype.hasUninferredWidth()) return emitError(loc, "extmodule port must have known width"); } @@ -5050,7 +5050,7 @@ ParseResult FIRCircuitParser::parseModule(CircuitOp circuit, bool isPublic, if (isPublic && version >= FIRVersion{4, 0, 0}) { for (auto [pi, loc] : llvm::zip_equal(portList, portLocs)) { - if (auto ftype = dyn_cast(pi.type)) { + if (auto ftype = type_dyn_cast(pi.type)) { if (ftype.hasUninferredWidth()) return emitError(loc, "public module port must have known width"); if (ftype.hasUninferredReset()) diff --git a/test/Dialect/FIRRTL/parse-errors.fir b/test/Dialect/FIRRTL/parse-errors.fir index 3ef306bd42b6..5c67e4707a5b 100644 --- a/test/Dialect/FIRRTL/parse-errors.fir +++ b/test/Dialect/FIRRTL/parse-errors.fir @@ -1348,7 +1348,6 @@ circuit Foo: ; expected-error @below {{expected layer name}} output a: Probe A.> - ;// ----- FIRRTL version 4.0.0 circuit UnknownWidthPublic: