From ccbb8ccf391457427ae239ef81cff51cb6e0cdbc Mon Sep 17 00:00:00 2001 From: Chris Leary Date: Thu, 14 Nov 2024 23:43:59 -0800 Subject: [PATCH 1/2] [VAST] `ExternPackageType` & tighten space invariants. Adds an `ExternPackageType` to refer to types defined in other packages (that we do not know the underlying bit width for or have a definition to refer to in the current `VerilogFile`). In the process found it was hard to reason about the invariants of `Emit()` giving back associated spaces for types and `EmitWithIdentifier()` and similar -- reworked that so all Emit functionality gives back a value with no whitespace to strip so the caller knows what to expect. Previously it was confusing/confused because DataKind could be empty string and DataType could also be empty string in various cases. Case in point this fixed an error in the test where there were two spaces after the `output` keyword. Also adds a bunch of tests for constructs we're about to use in stitching just to ensure they were all possible. --- xls/codegen/vast/vast.cc | 88 ++++++++++++--- xls/codegen/vast/vast.h | 52 +++++++-- xls/codegen/vast/vast_test.cc | 201 +++++++++++++++++++++++++++++++--- 3 files changed, 298 insertions(+), 43 deletions(-) diff --git a/xls/codegen/vast/vast.cc b/xls/codegen/vast/vast.cc index 6c2494deb7..2b33e09784 100644 --- a/xls/codegen/vast/vast.cc +++ b/xls/codegen/vast/vast.cc @@ -102,6 +102,21 @@ std::string EmitNothing(const VastNode* node, LineInfo* line_info) { return ""; } +// Helper for validating that there is not leading/trailing whitespace that can +// be stripped from string view `s` -- this helps us maintain our invariants +// that our Emit() calls generally do not return things with leading or +// trailing whitespace, so callers uniformly know what to expect in terms of +// spacing. +// +// This routine is made without performing string copies or performing a strcmp +// so should be usable in CHECKs instead of resorting to DCHECKs everywhere. +// +// i.e. callers are expected to call `CHECK(CannotStripWhitespace(s));` +bool CannotStripWhitespace(std::string_view s) { + std::string_view stripped = absl::StripAsciiWhitespace(s); + return stripped.size() == s.size(); +} + } // namespace int Precedence(OperatorKind kind) { @@ -301,6 +316,16 @@ std::string ToString(Direction direction) { } } +std::string DataType::EmitWithIdentifier(LineInfo* line_info, + std::string_view identifier) const { + std::string base = Emit(line_info); + if (base.empty()) { + return std::string{identifier}; + } + CHECK(CannotStripWhitespace(base)); + return absl::StrCat(base, " ", identifier); +} + std::string ScalarType::Emit(LineInfo* line_info) const { // The `DataKind` preceding the type is enough. if (!is_signed_) { @@ -308,7 +333,7 @@ std::string ScalarType::Emit(LineInfo* line_info) const { } LineInfoStart(line_info, this); LineInfoEnd(line_info, this); - return " signed"; + return "signed"; } std::string IntegerType::Emit(LineInfo* line_info) const { @@ -319,7 +344,7 @@ std::string IntegerType::Emit(LineInfo* line_info) const { } LineInfoStart(line_info, this); LineInfoEnd(line_info, this); - return " unsigned"; + return "unsigned"; } std::string MacroRef::Emit(LineInfo* line_info) const { @@ -500,17 +525,21 @@ LogicRef* VerilogFunction::return_value_ref() { std::string VerilogFunction::Emit(LineInfo* line_info) const { LineInfoStart(line_info, this); + + // Construct the return type for the function, sometimes we want to put + // "logic" in front when in SV mode. std::string return_type = return_value_def_->data_type()->EmitWithIdentifier(line_info, name()); - if (!absl::StartsWith(return_type, " ")) { - return_type = absl::StrCat(" ", return_type); - } if (return_value_def_->data_type()->IsScalar() && file()->use_system_verilog()) { // Preface the return type with "logic", so there's always a type provided. return_type = - absl::StrCat(" ", DataKindToString(DataKind::kLogic), return_type); + absl::StrCat(DataKindToString(DataKind::kLogic), " ", return_type); } + if (!return_type.empty()) { + return_type = absl::StrCat(" ", return_type); + } + std::string parameters = absl::StrJoin(argument_defs_, ", ", [=](std::string* out, Def* d) { absl::StrAppend(out, "input ", d->EmitNoSemi(line_info)); @@ -757,10 +786,11 @@ absl::StatusOr BitVectorType::FlatBitCountAsInt64() const { std::string BitVectorType::Emit(LineInfo* line_info) const { LineInfoStart(line_info, this); - std::string result = - absl::StrFormat("%s [%s:0]", is_signed_ ? " signed" : "", - size_expr_is_max_ ? size_expr_->Emit(line_info) - : WidthToLimit(line_info, size_expr_)); + std::string result = is_signed_ ? "signed " : ""; + absl::StrAppendFormat(&result, "[%s:0]", + size_expr_is_max_ + ? size_expr_->Emit(line_info) + : WidthToLimit(line_info, size_expr_)); LineInfoEnd(line_info, this); return result; } @@ -891,8 +921,20 @@ std::string Def::Emit(LineInfo* line_info) const { std::string Def::EmitNoSemi(LineInfo* line_info) const { LineInfoStart(line_info, this); std::string kind_str = DataKindToString(data_kind()); - std::string result = absl::StrCat( - kind_str, data_type()->EmitWithIdentifier(line_info, GetName())); + std::string data_type_str = + data_type()->EmitWithIdentifier(line_info, GetName()); + + CHECK(CannotStripWhitespace(kind_str)); + CHECK(CannotStripWhitespace(data_type_str)); + + std::string result; + if (kind_str.empty()) { + result = data_type_str; + } else if (data_type_str.empty()) { + result = kind_str; + } else { + result = absl::StrCat(kind_str, " ", data_type_str); + } LineInfoEnd(line_info, this); return result; } @@ -1106,8 +1148,10 @@ std::string Module::Emit(LineInfo* line_info) const { absl::StrAppend( &result, absl::StrJoin(ports_, ",\n ", [=](std::string* out, const Port& port) { + std::string wire_str = port.wire->EmitNoSemi(line_info); + CHECK(CannotStripWhitespace(wire_str)); absl::StrAppendFormat(out, "%s %s", ToString(port.direction), - port.wire->EmitNoSemi(line_info)); + wire_str); LineInfoIncrease(line_info, 1); })); absl::StrAppend(&result, "\n);\n"); @@ -1302,7 +1346,14 @@ std::string TypedefType::Emit(LineInfo* line_info) const { std::string ExternType::Emit(LineInfo* line_info) const { LineInfoStart(line_info, this); - std::string result = absl::StrCat(" ", name_); + std::string result = name_; + LineInfoEnd(line_info, this); + return result; +} + +std::string ExternPackageType::Emit(LineInfo* line_info) const { + LineInfoStart(line_info, this); + std::string result = absl::StrCat(package_name_, "::", type_name_); LineInfoEnd(line_info, this); return result; } @@ -1311,8 +1362,13 @@ std::string Enum::Emit(LineInfo* line_info) const { LineInfoStart(line_info, this); std::string result = "enum {\n"; if (kind_ != DataKind::kUntypedEnum) { - result = absl::StrFormat("enum %s%s {\n", DataKindToString(kind_), - BaseType()->Emit(line_info)); + std::string underlying_type_string = DataKindToString(kind_); + if (!underlying_type_string.empty()) { + absl::StrAppend(&underlying_type_string, " "); + } + absl::StrAppend(&underlying_type_string, BaseType()->Emit(line_info)); + + result = absl::StrFormat("enum %s {\n", underlying_type_string); } LineInfoIncrease(line_info, 1); for (int i = 0; i < members_.size(); i++) { diff --git a/xls/codegen/vast/vast.h b/xls/codegen/vast/vast.h index 17e5bcb602..a11adda85d 100644 --- a/xls/codegen/vast/vast.h +++ b/xls/codegen/vast/vast.h @@ -199,8 +199,8 @@ class DataType : public VastNode { // Returns whether this is a scalar signal type (for example, "wire foo"). virtual bool IsScalar() const { return false; } - // Returns whether this type represents to a typedef, struct, enum, or an - // array of a user-defined type. + // Returns whether this type represents a typedef, struct, enum, or an array + // of a user-defined type. virtual bool IsUserDefined() const { return false; } // Returns the width of the def (not counting packed or unpacked dimensions) @@ -233,9 +233,7 @@ class DataType : public VastNode { // This method is required rather than simply Emit because an identifier // string is nested within the string describing the type. virtual std::string EmitWithIdentifier(LineInfo* line_info, - std::string_view identifier) const { - return absl::StrFormat("%s %s", Emit(line_info), identifier); - } + std::string_view identifier) const; }; // Represents a scalar type. Example: @@ -507,7 +505,7 @@ class UserDefinedDef final : public Def { : Def(name, DataKind::kUser, data_type, init, file, loc) {} }; -// Register variable definition.Example: +// Register variable definition. Example: // reg [41:0] foo; class RegDef final : public Def { public: @@ -519,7 +517,7 @@ class RegDef final : public Def { : Def(name, DataKind::kReg, data_type, init, file, loc) {} }; -// Logic variable definition.Example: +// Logic variable definition. Example: // logic [41:0] foo; class LogicDef final : public Def { public: @@ -543,7 +541,7 @@ class UserDef final : public Def { : Def(name, DataKind::kUser, data_type, init, file, loc) {} }; -// Integer variable definition.Example: +// Integer variable definition. Example: // integer foo; class IntegerDef final : public Def { public: @@ -1163,6 +1161,37 @@ class Typedef final : public VastNode { Def* def_; }; +// A type that is defined in an external package, where we may not know the +// underlying bit vector count as we request in `ExternType`. +class ExternPackageType : public DataType { + public: + explicit ExternPackageType(std::string_view package_name, + std::string_view type_name, VerilogFile* file, + const SourceInfo& loc) + : DataType(file, loc), + package_name_(package_name), + type_name_(type_name) {} + + std::string Emit(LineInfo* line_info) const final; + + bool IsScalar() const final { return false; } + bool IsUserDefined() const final { return true; } + absl::StatusOr WidthAsInt64() const final { + return absl::UnimplementedError( + "WidthAsInt64 is not implemented for ExternPackageType."); + } + absl::StatusOr FlatBitCountAsInt64() const final { + return absl::UnimplementedError( + "FlatBitCountAsInt64 is not implemented for ExternPackageType."); + } + std::optional width() const final { return std::nullopt; } + bool is_signed() const final { return false; } + + private: + std::string package_name_; + std::string type_name_; +}; + // The type of an entity when its type is a typedef. This emits just the name of // the typedef. class TypedefType final : public UserDefinedAliasType { @@ -2299,7 +2328,12 @@ class VerilogPackageSection final : public VastNode { std::vector members_; }; -// Represents a package definition. +// Represents a package definition. Emits as: +// ``` +// package ${name}; +// .. content .. +// endpackage +// ``` class VerilogPackage final : public VastNode { public: VerilogPackage(std::string_view name, VerilogFile* file, diff --git a/xls/codegen/vast/vast_test.cc b/xls/codegen/vast/vast_test.cc index 09820f2c3c..8d5a477ff2 100644 --- a/xls/codegen/vast/vast_test.cc +++ b/xls/codegen/vast/vast_test.cc @@ -62,6 +62,24 @@ TEST_P(VastTest, SanitizeIdentifier) { EXPECT_EQ("add_1234", SanitizeIdentifier("add.1234")); } +TEST_P(VastTest, EmitScalarLogicDef) { + VerilogFile f(GetFileType()); + const SourceInfo si; + DataType* data_type = f.ScalarType(si); + LogicDef* logic_def = f.Make(si, "foo", data_type); + LineInfo line_info; + EXPECT_EQ(logic_def->Emit(&line_info), "logic foo;"); +} + +TEST_P(VastTest, EmitBitVectorLogicDef) { + VerilogFile f(GetFileType()); + const SourceInfo si; + DataType* data_type = f.BitVectorType(42, si); + LogicDef* logic_def = f.Make(si, "foo", data_type); + LineInfo line_info; + EXPECT_EQ(logic_def->Emit(&line_info), "logic [41:0] foo;"); +} + TEST_P(VastTest, DataTypes) { VerilogFile f(GetFileType()); @@ -72,7 +90,7 @@ TEST_P(VastTest, DataTypes) { EXPECT_THAT(scalar->FlatBitCountAsInt64(), IsOkAndHolds(1)); EXPECT_EQ(scalar->width(), std::nullopt); EXPECT_FALSE(scalar->is_signed()); - EXPECT_EQ(scalar->EmitWithIdentifier(&line_info, "foo"), " foo"); + EXPECT_EQ(scalar->EmitWithIdentifier(&line_info, "foo"), "foo"); EXPECT_EQ(line_info.LookupNode(scalar), std::make_optional(std::vector{LineSpan(0, 0)})); EXPECT_EQ(scalar->Emit(&line_info), ""); @@ -84,33 +102,33 @@ TEST_P(VastTest, DataTypes) { EXPECT_THAT(u1->FlatBitCountAsInt64(), IsOkAndHolds(1)); EXPECT_EQ(u1->width(), std::nullopt); EXPECT_FALSE(u1->is_signed()); - EXPECT_EQ(u1->EmitWithIdentifier(nullptr, "foo"), " foo"); + EXPECT_EQ(u1->EmitWithIdentifier(nullptr, "foo"), "foo"); EXPECT_EQ(u1->Emit(nullptr), ""); DataType* s1 = f.BitVectorType(1, SourceInfo(), /*is_signed=*/true); EXPECT_FALSE(s1->IsUserDefined()); - EXPECT_EQ(s1->EmitWithIdentifier(nullptr, "foo"), " signed foo"); + EXPECT_EQ(s1->EmitWithIdentifier(nullptr, "foo"), "signed foo"); EXPECT_THAT(s1->WidthAsInt64(), IsOkAndHolds(1)); EXPECT_THAT(s1->FlatBitCountAsInt64(), IsOkAndHolds(1)); EXPECT_TRUE(s1->is_signed()); DataType* u2 = f.BitVectorType(2, SourceInfo()); EXPECT_FALSE(u2->IsUserDefined()); - EXPECT_EQ(u2->EmitWithIdentifier(nullptr, "foo"), " [1:0] foo"); + EXPECT_EQ(u2->EmitWithIdentifier(nullptr, "foo"), "[1:0] foo"); EXPECT_THAT(u2->WidthAsInt64(), IsOkAndHolds(2)); EXPECT_THAT(u2->FlatBitCountAsInt64(), IsOkAndHolds(2)); EXPECT_FALSE(u2->is_signed()); DataType* u32 = f.BitVectorType(32, SourceInfo()); EXPECT_FALSE(u32->IsUserDefined()); - EXPECT_EQ(u32->EmitWithIdentifier(nullptr, "foo"), " [31:0] foo"); + EXPECT_EQ(u32->EmitWithIdentifier(nullptr, "foo"), "[31:0] foo"); EXPECT_THAT(u32->WidthAsInt64(), IsOkAndHolds(32)); EXPECT_THAT(u32->FlatBitCountAsInt64(), IsOkAndHolds(32)); EXPECT_FALSE(u32->is_signed()); DataType* s32 = f.BitVectorType(32, SourceInfo(), /*is_signed=*/true); EXPECT_FALSE(s32->IsUserDefined()); - EXPECT_EQ(s32->EmitWithIdentifier(nullptr, "foo"), " signed [31:0] foo"); + EXPECT_EQ(s32->EmitWithIdentifier(nullptr, "foo"), "signed [31:0] foo"); EXPECT_THAT(s32->WidthAsInt64(), IsOkAndHolds(32)); EXPECT_THAT(s32->FlatBitCountAsInt64(), IsOkAndHolds(32)); EXPECT_TRUE(s32->is_signed()); @@ -125,11 +143,11 @@ TEST_P(VastTest, DataTypes) { EXPECT_TRUE(packed_array->dims()[1]->IsLiteralWithValue(2)); EXPECT_FALSE(packed_array->IsUserDefined()); EXPECT_EQ(packed_array->EmitWithIdentifier(nullptr, "foo"), - " [9:0][2:0][1:0] foo"); + "[9:0][2:0][1:0] foo"); EXPECT_THAT(packed_array->WidthAsInt64(), IsOkAndHolds(10)); EXPECT_THAT(packed_array->FlatBitCountAsInt64(), IsOkAndHolds(60)); EXPECT_FALSE(packed_array->is_signed()); - EXPECT_EQ(packed_array->Emit(nullptr), " [9:0][2:0][1:0]"); + EXPECT_EQ(packed_array->Emit(nullptr), "[9:0][2:0][1:0]"); Enum* enum_def = f.Make(SourceInfo(), DataKind::kLogic, f.BitVectorType(32, SourceInfo())); @@ -218,20 +236,20 @@ TEST_P(VastTest, DataTypes) { } [5:0][2:0] foo)"); EXPECT_EQ(packed_array->EmitWithIdentifier(nullptr, "foo"), - " [9:0][2:0][1:0] foo"); + "[9:0][2:0][1:0] foo"); EXPECT_THAT(packed_array->WidthAsInt64(), IsOkAndHolds(10)); EXPECT_THAT(packed_array->FlatBitCountAsInt64(), IsOkAndHolds(60)); EXPECT_FALSE(packed_array->is_signed()); - EXPECT_EQ(packed_array->Emit(nullptr), " [9:0][2:0][1:0]"); + EXPECT_EQ(packed_array->Emit(nullptr), "[9:0][2:0][1:0]"); DataType* spacked_array = f.PackedArrayType(10, {3, 2}, SourceInfo(), /*is_signed=*/true); EXPECT_EQ(spacked_array->EmitWithIdentifier(nullptr, "foo"), - " signed [9:0][2:0][1:0] foo"); + "signed [9:0][2:0][1:0] foo"); EXPECT_THAT(spacked_array->WidthAsInt64(), IsOkAndHolds(10)); EXPECT_THAT(spacked_array->FlatBitCountAsInt64(), IsOkAndHolds(60)); EXPECT_TRUE(spacked_array->is_signed()); - EXPECT_EQ(spacked_array->Emit(nullptr), " signed [9:0][2:0][1:0]"); + EXPECT_EQ(spacked_array->Emit(nullptr), "signed [9:0][2:0][1:0]"); UnpackedArrayType* unpacked_array = f.UnpackedArrayType(10, {3, 2}, SourceInfo()); EXPECT_FALSE(unpacked_array->dims_are_max()); @@ -241,10 +259,10 @@ TEST_P(VastTest, DataTypes) { "3"); if (f.use_system_verilog()) { EXPECT_EQ(unpacked_array->EmitWithIdentifier(nullptr, "foo"), - " [9:0] foo[3][2]"); + "[9:0] foo[3][2]"); } else { EXPECT_EQ(unpacked_array->EmitWithIdentifier(nullptr, "foo"), - " [9:0] foo[0:2][0:1]"); + "[9:0] foo[0:2][0:1]"); } EXPECT_THAT(unpacked_array->WidthAsInt64(), IsOkAndHolds(10)); EXPECT_THAT(unpacked_array->FlatBitCountAsInt64(), IsOkAndHolds(60)); @@ -257,7 +275,7 @@ TEST_P(VastTest, DataTypes) { f.Mul(f.PlainLiteral(10, SourceInfo()), f.PlainLiteral(5, SourceInfo()), SourceInfo()), /*is_signed=*/false); - EXPECT_EQ(bv->EmitWithIdentifier(nullptr, "foo"), " [10 * 5 - 1:0] foo"); + EXPECT_EQ(bv->EmitWithIdentifier(nullptr, "foo"), "[10 * 5 - 1:0] foo"); ASSERT_TRUE(bv->width().has_value()); EXPECT_EQ((*bv->width())->Emit(nullptr), "10 * 5"); EXPECT_FALSE(bv->max().has_value()); @@ -276,7 +294,7 @@ TEST_P(VastTest, DataTypes) { f.Mul(f.PlainLiteral(10, SourceInfo()), f.PlainLiteral(5, SourceInfo()), SourceInfo()), /*is_signed=*/false, /*size_expr_is_max=*/true); - EXPECT_EQ(bv_max->EmitWithIdentifier(nullptr, "foo"), " [10 * 5:0] foo"); + EXPECT_EQ(bv_max->EmitWithIdentifier(nullptr, "foo"), "[10 * 5:0] foo"); EXPECT_FALSE(bv_max->width().has_value()); ASSERT_TRUE(bv_max->max().has_value()); EXPECT_EQ((*bv_max->max())->Emit(nullptr), "10 * 5"); @@ -482,14 +500,14 @@ TEST_P(VastTest, ModuleWithUserDataTypes) { if (UseSystemVerilog()) { EXPECT_EQ(module->Emit(nullptr), R"(module my_module( - output foobar out + output foobar out ); assign out = 64'h0000_0000_0000_0020; endmodule)"); } else { EXPECT_EQ(module->Emit(nullptr), R"(module my_module( - output foobar out + output foobar out ); assign out = 64'h0000_0000_0000_0020; endmodule)"); @@ -2104,6 +2122,153 @@ endpackage)"); std::vector{LineSpan(3, 3)}); } +// Tests that we can make an assignment of an enum variant to a wire or port. +TEST_P(VastTest, EnumAssignment) { + VerilogFile f(GetFileType()); + const SourceInfo si; + Module* m = f.AddModule("top", si); + + // Make a 2-bit enum with R (G & B elided) values. + Enum* enum_type = f.Make(si, DataKind::kLogic, f.BitVectorType(2, si)); + EnumMemberRef* red_ref = + enum_type->AddMember("RED", f.PlainLiteral(0, si), si); + + // Note that this is an externally defined type. + DataType* extern_enum_type = f.ExternType(enum_type, "color_e", si); + + LogicRef* signal = m->AddWire("signal", extern_enum_type, SourceInfo()); + m->Add(si, signal, red_ref); + + LineInfo line_info; + EXPECT_EQ(m->Emit(&line_info), + R"(module top; + wire color_e signal; + assign signal = RED; +endmodule)"); +} + +// Tests that we can declare ports that are extern (typedef) datatypes. +TEST_P(VastTest, ExternTypePort) { + VerilogFile f(GetFileType()); + const SourceInfo si; + Module* m = f.AddModule("top", si); + + // Make a 2-bit enum definition with RED (GREEN & BLUE elided) values. + Enum* enum_type = f.Make(si, DataKind::kLogic, f.BitVectorType(2, si)); + EnumMemberRef* red_ref = + enum_type->AddMember("RED", f.PlainLiteral(0, si), si); + + // Note that this is an externally defined type. + DataType* extern_enum_type = f.ExternType(enum_type, "color_e", si); + + // Declare a port of the extern type. + m->AddInput("input", extern_enum_type, si); + LogicRef* output = m->AddOutput("output", extern_enum_type, si); + + m->Add(si, output, red_ref); + + LineInfo line_info; + EXPECT_EQ(m->Emit(&line_info), + R"(module top( + input color_e input, + output color_e output +); + assign output = RED; +endmodule)"); +} + +// Tests that we can declare ports that are external-package references; e.g. +// we want to show we can make a port like: +// `input mypack::mystruct_t a` +TEST_P(VastTest, ExternalPackageTypePort) { + VerilogFile f(GetFileType()); + const SourceInfo si; + Module* m = f.AddModule("top", si); + + // Make an extern package type to use in the `input` construction. + auto* data_type = f.Make(si, "mypack", "mystruct_t"); + + m->AddInput("my_input", data_type, si); + + LineInfo line_info; + EXPECT_EQ(m->Emit(&line_info), + R"(module top( + input mypack::mystruct_t my_input +); + +endmodule)"); +} + +// Tests that we can declare ports that are packed arrays of external-package +// references; e.g. we want to show we can make a port like: +// `input mypack::mystruct_t [1:0][2:0][3:0] a` +TEST_P(VastTest, ExternalPackageTypePackedArrayPort) { + VerilogFile f(GetFileType()); + const SourceInfo si; + Module* m = f.AddModule("top", si); + + // Make an extern package type to use in the `input` construction. + auto* data_type = f.Make(si, "mypack", "mystruct_t"); + const std::vector packed_dims = {2, 3, 4}; + const bool dims_are_max = false; + auto* packed_array = + f.Make(si, data_type, packed_dims, dims_are_max); + + m->AddInput("my_input", packed_array, si); + + LineInfo line_info; + EXPECT_EQ(m->Emit(&line_info), + R"(module top( + input mypack::mystruct_t [1:0][2:0][3:0] my_input +); + +endmodule)"); +} + +// Tests that we can reference a slice of a multidimensional packed array on +// the LHS or RHS of an assign statement; e.g. +// ``` +// assign a[1][2][3:4] = b[1:0]; +// assign a[3:4] = c[2:1]; +// ``` +TEST_P(VastTest, SliceOfMultidimensionalPackedArrayOnLhsAndRhs) { + VerilogFile f(GetFileType()); + const SourceInfo si; + Module* m = f.AddModule("top", si); + + const std::vector dims = {3, 5}; + DataType* a_type = f.Make(si, 2, absl::MakeConstSpan(dims), + /*is_signed=*/false); + DataType* b_type = f.BitVectorType(2, si); + DataType* c_type = f.BitVectorType(3, si); + + LogicRef* a = m->AddWire("a", a_type, si); + LogicRef* b = m->AddWire("b", b_type, si); + LogicRef* c = m->AddWire("c", c_type, si); + + m->AddModuleMember(f.Make( + si, + f.Make( + si, + f.Make(si, f.Make(si, a, f.PlainLiteral(1, si)), + f.PlainLiteral(2, si)), + f.PlainLiteral(3, si), f.PlainLiteral(4, si)), + f.Make(si, b, f.PlainLiteral(1, si), f.PlainLiteral(0, si)))); + m->AddModuleMember(f.Make( + si, f.Make(si, a, f.PlainLiteral(3, si), f.PlainLiteral(4, si)), + f.Make(si, c, f.PlainLiteral(2, si), f.PlainLiteral(1, si)))); + + LineInfo line_info; + EXPECT_EQ(m->Emit(&line_info), + R"(module top; + wire [1:0][2:0][4:0] a; + wire [1:0] b; + wire [2:0] c; + assign a[1][2][3:4] = b[1:0]; + assign a[3:4] = c[2:1]; +endmodule)"); +} + INSTANTIATE_TEST_SUITE_P(VastTestInstantiation, VastTest, testing::Values(false, true), [](const testing::TestParamInfo& info) { From e56d590e9014a664b5911539d219f03a31725837 Mon Sep 17 00:00:00 2001 From: Chris Leary Date: Fri, 15 Nov 2024 10:40:10 -0800 Subject: [PATCH 2/2] Handle missing test case for integer underlying an enum. --- xls/codegen/vast/vast.cc | 43 ++++++++++++++++++++--------------- xls/codegen/vast/vast_test.cc | 34 +++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 18 deletions(-) diff --git a/xls/codegen/vast/vast.cc b/xls/codegen/vast/vast.cc index 2b33e09784..52cf290525 100644 --- a/xls/codegen/vast/vast.cc +++ b/xls/codegen/vast/vast.cc @@ -117,6 +117,25 @@ bool CannotStripWhitespace(std::string_view s) { return stripped.size() == s.size(); } +// Helper that combines DataKind and DataType strings -- since either of these +// can be empty under certain circumstances we consolidate the handling of +// inserting spaces appropriately in this routine. +std::string CombineKindAndDataType(std::string_view kind_str, + std::string_view data_type_str) { + CHECK(CannotStripWhitespace(kind_str)); + CHECK(CannotStripWhitespace(data_type_str)); + + std::string result; + if (kind_str.empty()) { + result = data_type_str; + } else if (data_type_str.empty()) { + result = kind_str; + } else { + result = absl::StrCat(kind_str, " ", data_type_str); + } + return result; +} + } // namespace int Precedence(OperatorKind kind) { @@ -923,18 +942,8 @@ std::string Def::EmitNoSemi(LineInfo* line_info) const { std::string kind_str = DataKindToString(data_kind()); std::string data_type_str = data_type()->EmitWithIdentifier(line_info, GetName()); + std::string result = CombineKindAndDataType(kind_str, data_type_str); - CHECK(CannotStripWhitespace(kind_str)); - CHECK(CannotStripWhitespace(data_type_str)); - - std::string result; - if (kind_str.empty()) { - result = data_type_str; - } else if (data_type_str.empty()) { - result = kind_str; - } else { - result = absl::StrCat(kind_str, " ", data_type_str); - } LineInfoEnd(line_info, this); return result; } @@ -1362,13 +1371,11 @@ std::string Enum::Emit(LineInfo* line_info) const { LineInfoStart(line_info, this); std::string result = "enum {\n"; if (kind_ != DataKind::kUntypedEnum) { - std::string underlying_type_string = DataKindToString(kind_); - if (!underlying_type_string.empty()) { - absl::StrAppend(&underlying_type_string, " "); - } - absl::StrAppend(&underlying_type_string, BaseType()->Emit(line_info)); - - result = absl::StrFormat("enum %s {\n", underlying_type_string); + std::string kind_str = DataKindToString(kind_); + std::string data_type_str = BaseType()->Emit(line_info); + std::string underlying_type_str = + CombineKindAndDataType(kind_str, data_type_str); + result = absl::StrFormat("enum %s {\n", underlying_type_str); } LineInfoIncrease(line_info, 1); for (int i = 0; i < members_.size(); i++) { diff --git a/xls/codegen/vast/vast_test.cc b/xls/codegen/vast/vast_test.cc index 8d5a477ff2..a6b0fae18c 100644 --- a/xls/codegen/vast/vast_test.cc +++ b/xls/codegen/vast/vast_test.cc @@ -80,6 +80,40 @@ TEST_P(VastTest, EmitBitVectorLogicDef) { EXPECT_EQ(logic_def->Emit(&line_info), "logic [41:0] foo;"); } +// Tests we can build and emit a construct of the following form: +// ``` +// typedef enum integer { +// kElem0 = 0, +// kElem1 = 1, +// kElem2 = 2, +// kElem3 = 3, +// } enum_t; +// ``` +TEST_P(VastTest, EmitTypedefOfEnumWithUnderlyingInteger) { + VerilogFile f(GetFileType()); + const SourceInfo si; + DataType* data_type = f.IntegerType(si); + Enum* enum_def = f.Make(si, DataKind::kInteger, data_type); + EnumMemberRef* elem0 = + enum_def->AddMember("kElem0", f.PlainLiteral(0, si), si); + EnumMemberRef* elem1 = + enum_def->AddMember("kElem1", f.PlainLiteral(1, si), si); + EnumMemberRef* elem2 = + enum_def->AddMember("kElem2", f.PlainLiteral(2, si), si); + EnumMemberRef* elem3 = + enum_def->AddMember("kElem3", f.PlainLiteral(3, si), si); + Typedef* enum_typedef = + f.Make(si, f.Make(si, "enum_t", DataKind::kUser, enum_def)); + LineInfo line_info; + EXPECT_EQ(enum_typedef->Emit(&line_info), + R"(typedef enum integer { + kElem0 = 0, + kElem1 = 1, + kElem2 = 2, + kElem3 = 3 +} enum_t;)"); +} + TEST_P(VastTest, DataTypes) { VerilogFile f(GetFileType());