From f617ad36018cef51fa521608c695d28c4d83bc66 Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 03:22:49 +0000 Subject: [PATCH 1/7] Emit hw.typealias for named types in CIRCT IR output Register named Kanagawa types (structs, unions, enums) as hw::TypedeclOp entries in the TypeScopeOp and replace anonymous hw::StructType / hw::UnionType uses with hw::TypeAliasType references on ESI-visible ports. This ensures ESI-generated C++ headers carry meaningful type names instead of anonymous struct layouts. Changes: - Add RegisterNamedType() and GetTypeAlias() to ModuleDeclarationHelper to build a Type* -> TypeAliasType cache. - Add ToMlirTypeAliased() which mirrors ToMlirType() but substitutes registered type aliases at every recursion level. - In DeclareCore(), register all exported types before port declaration. - In DeclareCorePorts(), use ToMlirTypeAliased for export and extern function ports (parameters and return types). - In EmitEsiWrapper(), use ToMlirTypeAliased for ESI channel payload types so bundle ports carry named type aliases. - Add C++ unit test (TypeAliasTest) in internal_tests.cpp verifying RegisterNamedType/ToMlirTypeAliased produce hw::TypeAliasType. - Add end-to-end test compiling typealias_test.k with --write-circt-ir and verifying hw.typedecl/@typealias patterns in the MLIR output. AI-assisted-by: GitHub Copilot --- .github/copilot-instructions.md | 5 ++ compiler/cpp/circt_util.cpp | 124 ++++++++++++++++++++++++++++++-- compiler/cpp/circt_util.h | 12 +++- compiler/cpp/internal_tests.cpp | 74 +++++++++++++++++++ compiler/cpp/verilog.cpp | 26 +++++-- test/compiler/CMakeLists.txt | 62 +++++++++++++++- test/compiler/typealias_test.k | 52 ++++++++++++++ 7 files changed, 344 insertions(+), 11 deletions(-) create mode 100644 .github/copilot-instructions.md create mode 100644 test/compiler/typealias_test.k diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..328589c0 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,5 @@ +# Instructions for GitHub Copilot + +- Refer to `doc/compiler-design-reference.md` to learn about the design of the compiler. +- Other docs in that same directory may also be helpful for understanding the language itself. +- Use BUILDING.md to understand how to build the project and run tests. diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index d040af38..59f553a8 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -579,6 +579,57 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) } } +mlir::Type ToMlirTypeAliased(const Type *typeIn, bool signedness, ModuleDeclarationHelper &helper) +{ + // Check if this type has a registered alias + auto alias = helper.GetTypeAlias(typeIn); + if (alias) + { + return *alias; + } + + // For struct types, recursively use aliased versions for member types + const StructUnionType *structUnionType = dynamic_cast(typeIn); + if (structUnionType) + { + if (structUnionType->_type == ContainerType::Struct) + { + llvm::SmallVector fields; + for (const StructUnionType::EntryType &member : structUnionType->_members) + { + const Type *const memberType = member.second->GetDeclaredType(); + fields.push_back(circt::hw::StructType::FieldInfo{StringToStringAttr(member.first), + ToMlirTypeAliased(memberType, signedness, helper)}); + } + std::reverse(fields.begin(), fields.end()); + return circt::hw::StructType::get(g_compiler->GetMlirContext(), fields); + } + else + { + llvm::SmallVector fields; + for (const StructUnionType::EntryType &member : structUnionType->_members) + { + const Type *const memberType = member.second->GetDeclaredType(); + fields.push_back(circt::hw::UnionType::FieldInfo{StringToStringAttr(member.first), + ToMlirTypeAliased(memberType, signedness, helper)}); + } + std::reverse(fields.begin(), fields.end()); + return circt::hw::UnionType::get(g_compiler->GetMlirContext(), fields); + } + } + + // For array types, recursively use aliased versions for element type + const ArrayType *arrayType = dynamic_cast(typeIn); + if (arrayType) + { + return GetPackedArrayType(ToMlirTypeAliased(arrayType->_elementType, signedness, helper), + arrayType->_arraySize); + } + + // For all other types, fall back to the non-aliased version + return ToMlirType(typeIn, signedness); +} + // Used to avoid symbol name conflicts for elements like container ports // returns a symbol name which will be unique provided // that flattened container paths are unique @@ -1720,7 +1771,7 @@ void ModuleDeclarationHelper::AssertStructsMatch(const mlir::Type &circtTypeAlia _verbatimBuffer.Str() << "end"; } -mlir::Type ModuleDeclarationHelper::GetTypeAlias(const std::string &name, const mlir::Type &referencedType) +mlir::Type ModuleDeclarationHelper::CreateTypeAlias(const std::string &name, const mlir::Type &referencedType) { // AddTypedefs must be called first assert(_typeScopeOp); @@ -1731,10 +1782,75 @@ mlir::Type ModuleDeclarationHelper::GetTypeAlias(const std::string &name, const return circt::hw::TypeAliasType::get(symbolRefAttr, referencedType); } +void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) +{ + assert(_typeScopeOp); + + // Skip if already registered + if (_typeAliasCache.count(kanagawaType)) + { + return; + } + + const StructUnionType *structUnionType = dynamic_cast(kanagawaType); + const EnumType *enumType = dynamic_cast(kanagawaType); + + std::string typeName; + if (structUnionType) + { + typeName = structUnionType->GetName(); + } + else if (enumType) + { + typeName = enumType->GetName(); + } + + // Only register types with a non-empty name + if (typeName.empty()) + { + return; + } + + // Recursively register member types first (for structs/unions) + if (structUnionType) + { + for (const StructUnionType::EntryType &member : structUnionType->_members) + { + const Type *memberType = member.second->GetDeclaredType(); + RegisterNamedType(memberType); + } + } + + // Build the MLIR type using the alias-aware conversion so inner named types use aliases + mlir::Type mlirType = ToMlirTypeAliased(kanagawaType, false, *this); + + // Create the TypedeclOp in the TypeScope block + { + circt::OpBuilder::InsertionGuard g(_opb); + _opb.setInsertionPointToEnd(_typeScopeOp.getBodyBlock()); + circt::hw::TypedeclOp::create(_opb, _location, StringToStringAttr(typeName), mlirType, + StringToStringAttr(typeName)); + } + + // Create and cache the type alias + mlir::Type aliasType = CreateTypeAlias(typeName, mlirType); + _typeAliasCache[kanagawaType] = aliasType; +} + +std::optional ModuleDeclarationHelper::GetTypeAlias(const Type *kanagawaType) const +{ + auto it = _typeAliasCache.find(kanagawaType); + if (it != _typeAliasCache.end()) + { + return it->second; + } + return std::nullopt; +} + mlir::Type ModuleDeclarationHelper::GetInspectableTypeAlias() { assert(GetCodeGenConfig()._inspection); - return GetTypeAlias(InspectableValueName, GetInspectableStructType()); + return CreateTypeAlias(InspectableValueName, GetInspectableStructType()); } mlir::ModuleOp ModuleDeclarationHelper::MlirModule() { return _mlirModule; } @@ -1884,7 +2000,7 @@ void ModuleDeclarationHelper::EmitEsiWrapper(const std::string &circtDesignName) break; case EsiPortSemantics::Payload: - bundlePayloadTypes.push_back(ToMlirType(portInfo._origType, true)); + bundlePayloadTypes.push_back(ToMlirTypeAliased(portInfo._origType, true, *this)); payloadTypes.push_back(portInfo._hwPortInfo.type); payloadNames.push_back(portInfo._hwPortInfo.name.str()); payloadFieldNames.push_back(portInfo._fieldName); @@ -2035,7 +2151,7 @@ void ModuleDeclarationHelper::EmitEsiWrapper(const std::string &circtDesignName) payload.push_back(ReadContainerPort( _opb, _location, pathToContainer, GetFullyQualifiedStringAttr(ObjectPath(), portInfo._hwPortInfo.name.str()), - portInfo._hwPortInfo.type, ToMlirType(portInfo._origType, true))); + portInfo._hwPortInfo.type, ToMlirTypeAliased(portInfo._origType, true, *this))); } break; diff --git a/compiler/cpp/circt_util.h b/compiler/cpp/circt_util.h index 32726709..40d278e0 100644 --- a/compiler/cpp/circt_util.h +++ b/compiler/cpp/circt_util.h @@ -86,6 +86,9 @@ circt::seq::ClockType GetClockType(); mlir::Type ToMlirType(const Type* typeIn, bool signedness = false); +class ModuleDeclarationHelper; +mlir::Type ToMlirTypeAliased(const Type* typeIn, bool signedness, ModuleDeclarationHelper& helper); + mlir::Value GetTypedZeros(circt::OpBuilder& opb, const mlir::Location& location, const mlir::Type& typeIn); mlir::Value PopCount(circt::OpBuilder& opb, const mlir::Location location, const mlir::ValueRange values, @@ -316,6 +319,10 @@ class ModuleDeclarationHelper void AddTypedefs(const std::string& typeScopeName); + void RegisterNamedType(const Type* kanagawaType); + + std::optional GetTypeAlias(const Type* kanagawaType) const; + mlir::Type GetInspectableTypeAlias(); mlir::ModuleOp MlirModule(); @@ -350,7 +357,7 @@ class ModuleDeclarationHelper const std::string& circtDesignName, const mlir::Type type); private: - mlir::Type GetTypeAlias(const std::string& name, const mlir::Type& referencedType); + mlir::Type CreateTypeAlias(const std::string& name, const mlir::Type& referencedType); private: void AssertStructsMatch(const mlir::Type& circtTypeAlias, const std::string& otherStructName); @@ -395,6 +402,9 @@ class ModuleDeclarationHelper circt::hw::TypeScopeOp _typeScopeOp; + // Maps Kanagawa Type* to hw::TypeAliasType for named types + std::map _typeAliasCache; + bool _finished; bool _exportVerilog; diff --git a/compiler/cpp/internal_tests.cpp b/compiler/cpp/internal_tests.cpp index c596d9a1..5f344a3b 100644 --- a/compiler/cpp/internal_tests.cpp +++ b/compiler/cpp/internal_tests.cpp @@ -2502,6 +2502,78 @@ void ArrayTypeStringTest() TestAssert("([[memory]] uint8[4])[64][32]" == mem_u8_64_32_4->GetName()); } +// Test that RegisterNamedType creates hw::TypeAliasType for named types +// and that ToMlirTypeAliased returns the alias for registered types +void TypeAliasTest() +{ + // Create a RedirectableSourceWriter for ModuleDeclarationHelper + class TestSourceWriter : public RedirectableSourceWriter + { + protected: + std::ostream& GetStreamImpl() override { return _str; } + + private: + std::ostringstream _str; + }; + + Location loc = {}; + const LeafType* const u8Type = g_compiler->GetLeafType(BaseType::Uint, 8, loc); + const LeafType* const u16Type = g_compiler->GetLeafType(BaseType::Uint, 16, loc); + + // Create an EnumType (no DeclareNode needed) + std::vector enumConstants = {{"Red", 0}, {"Green", 1}, {"Blue", 2}}; + EnumType testEnum("TestColor", u8Type, enumConstants, loc); + + // Create an MLIR module and ModuleDeclarationHelper + TestSourceWriter writer; + mlir::ModuleOp mlirModule = CreateMlirModuleAndDesign(GetUnknownLocation(), "TestDesign"); + ModuleDeclarationHelper helper(writer, "TestModule", GetUnknownLocation(), "TestDesign", &mlirModule); + helper.AddTypedefs("TestTypeScope"); + + // Before registration, ToMlirTypeAliased should return the same as ToMlirType + mlir::Type plainType = ToMlirType(&testEnum); + mlir::Type aliasedBefore = ToMlirTypeAliased(&testEnum, false, helper); + TestAssert(!llvm::isa(aliasedBefore)); + + // Register the enum type + helper.RegisterNamedType(&testEnum); + + // After registration, GetTypeAlias should return a TypeAliasType + auto alias = helper.GetTypeAlias(&testEnum); + TestAssert(alias.has_value()); + TestAssert(llvm::isa(*alias)); + + // The alias should wrap the same inner type + circt::hw::TypeAliasType aliasType = llvm::cast(*alias); + TestAssert(aliasType.getInnerType() == plainType); + TestAssertEqual(std::string("TestColor"), aliasType.getRef().getLeafReference().str()); + + // ToMlirTypeAliased should now return the alias + mlir::Type aliasedAfter = ToMlirTypeAliased(&testEnum, false, helper); + TestAssert(llvm::isa(aliasedAfter)); + + // Registering the same type twice should be a no-op + helper.RegisterNamedType(&testEnum); + auto alias2 = helper.GetTypeAlias(&testEnum); + TestAssert(alias2.has_value()); + TestAssert(*alias == *alias2); + + // A plain LeafType (not named) should not get an alias + helper.RegisterNamedType(u16Type); + auto noAlias = helper.GetTypeAlias(u16Type); + TestAssert(!noAlias.has_value()); + + // Verify the MLIR module can be printed (basic sanity) + std::string mlirStr; + llvm::raw_string_ostream os(mlirStr); + mlirModule.print(os); + TestAssert(mlirStr.find("hw.typedecl @TestColor") != std::string::npos); + TestAssert(mlirStr.find("TestTypeScope") != std::string::npos); + + // Clean up - erase the module to avoid leaks + mlirModule->erase(); +} + int InternalTests() { int result = -1; @@ -2538,6 +2610,8 @@ int InternalTests() ArrayTypeStringTest(); + TypeAliasTest(); + FixupLutTest(); LutOperationsProduceSameResultTest(); diff --git a/compiler/cpp/verilog.cpp b/compiler/cpp/verilog.cpp index 7e5fb98b..d60d7331 100644 --- a/compiler/cpp/verilog.cpp +++ b/compiler/cpp/verilog.cpp @@ -6562,8 +6562,9 @@ class VerilogCompiler coreModule.AddPort(port._name, port._input ? circt::hw::ModulePort::Direction::Input : circt::hw::ModulePort::Direction::Output, - ToMlirType(port._type), port._type, port._portSemantics, port._channelSemantics, - port._channelName, port._fieldName); + ToMlirTypeAliased(port._type, false, coreModule), port._type, + port._portSemantics, port._channelSemantics, port._channelName, + port._fieldName); } } @@ -6628,7 +6629,8 @@ class VerilogCompiler { const Type *type = functionNode->GetParameterType(i); coreModule.AddPort(prefix + "_" + functionNode->GetParameterName(i) + "_out", - circt::hw::ModulePort::Direction::Output, ToMlirType(type), type, + circt::hw::ModulePort::Direction::Output, + ToMlirTypeAliased(type, false, coreModule), type, EsiPortSemantics::Payload, EsiChannelSemantics::FromGeneratedHw, EsiChannelName::Arguments, functionNode->GetParameterName(i)); } @@ -6652,8 +6654,9 @@ class VerilogCompiler { const Type *type = functionNode->GetReturnType(); coreModule.AddPort(prefix + "_result_in", circt::hw::ModulePort::Direction::Input, - ToMlirType(type), type, EsiPortSemantics::Payload, - EsiChannelSemantics::ToGeneratedHw, EsiChannelName::Results); + ToMlirTypeAliased(type, false, coreModule), type, + EsiPortSemantics::Payload, EsiChannelSemantics::ToGeneratedHw, + EsiChannelName::Results); } if (!isNoBackpressure) @@ -7109,6 +7112,19 @@ class VerilogCompiler // to ensure that the generate `ifndef _TYPESCOPE_* macros all agree coreModule.AddTypedefs("CoreModuleTypeScope"); + // Register named types as type aliases in the CIRCT IR type scope + // so that port types and ESI channel payloads use named type aliases + // _exportedTypes is already topologically sorted by SortExportedTypes() + for (const Type *const type : _program._exportedTypes) + { + coreModule.RegisterNamedType(type); + } + + for (const auto &t : _program._exportedTypedefs) + { + coreModule.RegisterNamedType(t.second); + } + // Input and outputs of the KanagawaCore module DeclareCorePorts(coreModule, resetReplicas); diff --git a/test/compiler/CMakeLists.txt b/test/compiler/CMakeLists.txt index 0b4e5a7d..d51ee2b2 100644 --- a/test/compiler/CMakeLists.txt +++ b/test/compiler/CMakeLists.txt @@ -40,11 +40,71 @@ add_test( # Phoney target to ensure compiler test dependencies have been built add_custom_target(compiler_tests DEPENDS compiler_internal_test) +# End-to-end test: compile a .k file with named types and verify the CIRCT IR +# output contains hw.typedecl and hw.typealias entries. +# Uses add_kanagawa() to compile, then cmake -P to grep the MLIR output. +set(_typealias_outdir "${CMAKE_CURRENT_BINARY_DIR}/typealias_test_output") + +add_kanagawa(typealias_test_hls + SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/typealias_test.k" + OPTIONS --backend=sv --write-circt-ir --import-dir=${CMAKE_SOURCE_DIR}/library + OUTPUT_DIR "${_typealias_outdir}" +) + +# CMake script that checks the generated MLIR file for expected patterns +file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/check_typealias.cmake" [=[ +# Find the .mlir file in the output directory +file(GLOB MLIR_FILES "${OUTDIR}/*.mlir") +list(LENGTH MLIR_FILES NUM_MLIR) +if(NUM_MLIR EQUAL 0) + message(FATAL_ERROR "No .mlir file found in ${OUTDIR}") +endif() +list(GET MLIR_FILES 0 MLIR_FILE) +file(READ "${MLIR_FILE}" MLIR_CONTENT) +message(STATUS "Checking ${MLIR_FILE} for type aliases...") + +macro(check_pattern DESC PATTERN) + string(REGEX MATCH "${PATTERN}" _match "${MLIR_CONTENT}") + if(_match) + message(STATUS " PASS: ${DESC}") + else() + message(FATAL_ERROR " FAIL: ${DESC} (pattern: ${PATTERN})") + endif() +endmacro() + +check_pattern("TypeScope contains Point typedecl" "hw[.]typedecl @Point") +check_pattern("TypeScope contains Line typedecl" "hw[.]typedecl @Line") +check_pattern("TypeScope contains Color typedecl" "hw[.]typedecl @Color") +check_pattern("Core port uses Point type alias" "typealias<@CoreModuleTypeScope::@Point") +check_pattern("Core port uses Line type alias" "typealias<@CoreModuleTypeScope::@Line") +check_pattern("EsiWrapper container exists" "EsiWrapper") +check_pattern("ESI channel uses Color type alias" "typealias<@CoreModuleTypeScope::@Color") +message(STATUS "All type alias checks passed.") +]=]) + +add_test( + NAME compiler.typealias + COMMAND ${CMAKE_COMMAND} + -DOUTDIR=${_typealias_outdir} + -P "${CMAKE_CURRENT_BINARY_DIR}/check_typealias.cmake" +) +# The .k must be compiled before the test can run +set_tests_properties(compiler.typealias PROPERTIES + DEPENDS typealias_test_hls + FIXTURES_REQUIRED typealias_test_hls_fixture +) +# Register the HLS compilation as a fixture-setup test +add_test( + NAME compiler.typealias_setup + COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target typealias_test_hls +) +set_tests_properties(compiler.typealias_setup PROPERTIES FIXTURES_SETUP typealias_test_hls_fixture) + # Add a target to run all compiler tests add_custom_target(run_compiler_tests COMMAND ${CMAKE_CTEST_COMMAND} --test-dir ${CMAKE_CURRENT_BINARY_DIR} -R "^compiler\\." --output-on-failure --verbose WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} - DEPENDS compiler_internal_test + DEPENDS compiler_internal_test typealias_test_hls COMMENT "Run all compiler internal tests" USES_TERMINAL ) diff --git a/test/compiler/typealias_test.k b/test/compiler/typealias_test.k new file mode 100644 index 00000000..7af4c10b --- /dev/null +++ b/test/compiler/typealias_test.k @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Test that named struct and enum types are emitted as hw.typealias +// in the CIRCT IR output, both on core module ports and ESI wrapper ports. + +struct Point +{ + uint16 x; + uint16 y; +} + +struct Line +{ + Point start; + Point end; +} + +enum Color : uint8 +{ + Red, + Green, + Blue +} + +class TypeAliasTestModule +{ +public: + // Exported function with a named struct parameter and return + Point TranslatePoint(Point p, uint16 dx, uint16 dy) + { + Point result = { cast(p.x + dx), cast(p.y + dy) }; + return result; + } + + // Exported function with nested named struct parameter + Line TranslateLine(Line l, uint16 dx, uint16 dy) + { + Point s = { cast(l.start.x + dx), cast(l.start.y + dy) }; + Point e = { cast(l.end.x + dx), cast(l.end.y + dy) }; + Line result = { s, e }; + return result; + } + + // Exported function with enum type + uint8 GetColorValue(Color c) + { + return cast(c); + } +} + +export TypeAliasTestModule; From b465bcc7869f5f3aaaaf44cb46b1474db58b861a Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 03:51:54 +0000 Subject: [PATCH 2/7] Address PR review comments - Fix potential use-after-free: wrap ModuleDeclarationHelper in an inner scope in TypeAliasTest so it is destroyed before mlirModule->erase(). - Add name-based dedup to RegisterNamedType: if a distinct Type* has the same name as an already-registered type, reuse the existing TypeAliasType instead of creating a duplicate hw.typedecl symbol. New _typeAliasByName map tracks name -> alias alongside the existing Type* -> alias cache. - Fix CMake test flakiness: use CLEAN_OUTPUT_DIR on add_kanagawa to prevent stale .mlir files, and fail if the glob finds != 1 .mlir file (with a diagnostic listing the candidates). - Remove incorrect DEPENDS on build target from set_tests_properties; rely solely on the FIXTURES_REQUIRED/FIXTURES_SETUP mechanism. --- compiler/cpp/circt_util.cpp | 10 ++++ compiler/cpp/circt_util.h | 4 ++ compiler/cpp/internal_tests.cpp | 92 +++++++++++++++++---------------- test/compiler/CMakeLists.txt | 8 ++- 4 files changed, 69 insertions(+), 45 deletions(-) diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index 59f553a8..57b5a7de 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -1811,6 +1811,15 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) return; } + // Check if a different Type* with the same name was already registered. + // Reuse the existing alias to prevent duplicate hw.typedecl symbols. + auto nameIt = _typeAliasByName.find(typeName); + if (nameIt != _typeAliasByName.end()) + { + _typeAliasCache[kanagawaType] = nameIt->second; + return; + } + // Recursively register member types first (for structs/unions) if (structUnionType) { @@ -1835,6 +1844,7 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) // Create and cache the type alias mlir::Type aliasType = CreateTypeAlias(typeName, mlirType); _typeAliasCache[kanagawaType] = aliasType; + _typeAliasByName[typeName] = aliasType; } std::optional ModuleDeclarationHelper::GetTypeAlias(const Type *kanagawaType) const diff --git a/compiler/cpp/circt_util.h b/compiler/cpp/circt_util.h index 40d278e0..c505b717 100644 --- a/compiler/cpp/circt_util.h +++ b/compiler/cpp/circt_util.h @@ -405,6 +405,10 @@ class ModuleDeclarationHelper // Maps Kanagawa Type* to hw::TypeAliasType for named types std::map _typeAliasCache; + // Maps type name to TypeAliasType to prevent duplicate TypedeclOps + // when distinct Type* pointers share the same name + std::map _typeAliasByName; + bool _finished; bool _exportVerilog; diff --git a/compiler/cpp/internal_tests.cpp b/compiler/cpp/internal_tests.cpp index 5f344a3b..502f3e8b 100644 --- a/compiler/cpp/internal_tests.cpp +++ b/compiler/cpp/internal_tests.cpp @@ -2527,50 +2527,54 @@ void TypeAliasTest() // Create an MLIR module and ModuleDeclarationHelper TestSourceWriter writer; mlir::ModuleOp mlirModule = CreateMlirModuleAndDesign(GetUnknownLocation(), "TestDesign"); - ModuleDeclarationHelper helper(writer, "TestModule", GetUnknownLocation(), "TestDesign", &mlirModule); - helper.AddTypedefs("TestTypeScope"); - - // Before registration, ToMlirTypeAliased should return the same as ToMlirType - mlir::Type plainType = ToMlirType(&testEnum); - mlir::Type aliasedBefore = ToMlirTypeAliased(&testEnum, false, helper); - TestAssert(!llvm::isa(aliasedBefore)); - - // Register the enum type - helper.RegisterNamedType(&testEnum); - - // After registration, GetTypeAlias should return a TypeAliasType - auto alias = helper.GetTypeAlias(&testEnum); - TestAssert(alias.has_value()); - TestAssert(llvm::isa(*alias)); - - // The alias should wrap the same inner type - circt::hw::TypeAliasType aliasType = llvm::cast(*alias); - TestAssert(aliasType.getInnerType() == plainType); - TestAssertEqual(std::string("TestColor"), aliasType.getRef().getLeafReference().str()); - - // ToMlirTypeAliased should now return the alias - mlir::Type aliasedAfter = ToMlirTypeAliased(&testEnum, false, helper); - TestAssert(llvm::isa(aliasedAfter)); - - // Registering the same type twice should be a no-op - helper.RegisterNamedType(&testEnum); - auto alias2 = helper.GetTypeAlias(&testEnum); - TestAssert(alias2.has_value()); - TestAssert(*alias == *alias2); - - // A plain LeafType (not named) should not get an alias - helper.RegisterNamedType(u16Type); - auto noAlias = helper.GetTypeAlias(u16Type); - TestAssert(!noAlias.has_value()); - - // Verify the MLIR module can be printed (basic sanity) - std::string mlirStr; - llvm::raw_string_ostream os(mlirStr); - mlirModule.print(os); - TestAssert(mlirStr.find("hw.typedecl @TestColor") != std::string::npos); - TestAssert(mlirStr.find("TestTypeScope") != std::string::npos); - - // Clean up - erase the module to avoid leaks + + // Scope the helper so it is destroyed before we erase the module + { + ModuleDeclarationHelper helper(writer, "TestModule", GetUnknownLocation(), "TestDesign", &mlirModule); + helper.AddTypedefs("TestTypeScope"); + + // Before registration, ToMlirTypeAliased should return the same as ToMlirType + mlir::Type plainType = ToMlirType(&testEnum); + mlir::Type aliasedBefore = ToMlirTypeAliased(&testEnum, false, helper); + TestAssert(!llvm::isa(aliasedBefore)); + + // Register the enum type + helper.RegisterNamedType(&testEnum); + + // After registration, GetTypeAlias should return a TypeAliasType + auto alias = helper.GetTypeAlias(&testEnum); + TestAssert(alias.has_value()); + TestAssert(llvm::isa(*alias)); + + // The alias should wrap the same inner type + circt::hw::TypeAliasType aliasType = llvm::cast(*alias); + TestAssert(aliasType.getInnerType() == plainType); + TestAssertEqual(std::string("TestColor"), aliasType.getRef().getLeafReference().str()); + + // ToMlirTypeAliased should now return the alias + mlir::Type aliasedAfter = ToMlirTypeAliased(&testEnum, false, helper); + TestAssert(llvm::isa(aliasedAfter)); + + // Registering the same type twice should be a no-op + helper.RegisterNamedType(&testEnum); + auto alias2 = helper.GetTypeAlias(&testEnum); + TestAssert(alias2.has_value()); + TestAssert(*alias == *alias2); + + // A plain LeafType (not named) should not get an alias + helper.RegisterNamedType(u16Type); + auto noAlias = helper.GetTypeAlias(u16Type); + TestAssert(!noAlias.has_value()); + + // Verify the MLIR module can be printed (basic sanity) + std::string mlirStr; + llvm::raw_string_ostream os(mlirStr); + mlirModule.print(os); + TestAssert(mlirStr.find("hw.typedecl @TestColor") != std::string::npos); + TestAssert(mlirStr.find("TestTypeScope") != std::string::npos); + } + + // Clean up - helper is out of scope, safe to erase the module mlirModule->erase(); } diff --git a/test/compiler/CMakeLists.txt b/test/compiler/CMakeLists.txt index d51ee2b2..4b171452 100644 --- a/test/compiler/CMakeLists.txt +++ b/test/compiler/CMakeLists.txt @@ -49,16 +49,23 @@ add_kanagawa(typealias_test_hls SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/typealias_test.k" OPTIONS --backend=sv --write-circt-ir --import-dir=${CMAKE_SOURCE_DIR}/library OUTPUT_DIR "${_typealias_outdir}" + CLEAN_OUTPUT_DIR ) # CMake script that checks the generated MLIR file for expected patterns file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/check_typealias.cmake" [=[ # Find the .mlir file in the output directory file(GLOB MLIR_FILES "${OUTDIR}/*.mlir") +list(SORT MLIR_FILES) list(LENGTH MLIR_FILES NUM_MLIR) if(NUM_MLIR EQUAL 0) message(FATAL_ERROR "No .mlir file found in ${OUTDIR}") endif() +if(NOT NUM_MLIR EQUAL 1) + string(JOIN "\n " MLIR_FILE_LIST ${MLIR_FILES}) + message(FATAL_ERROR + "Expected exactly one .mlir file in ${OUTDIR}, found ${NUM_MLIR}:\n ${MLIR_FILE_LIST}") +endif() list(GET MLIR_FILES 0 MLIR_FILE) file(READ "${MLIR_FILE}" MLIR_CONTENT) message(STATUS "Checking ${MLIR_FILE} for type aliases...") @@ -90,7 +97,6 @@ add_test( ) # The .k must be compiled before the test can run set_tests_properties(compiler.typealias PROPERTIES - DEPENDS typealias_test_hls FIXTURES_REQUIRED typealias_test_hls_fixture ) # Register the HLS compilation as a fixture-setup test From b5c16d04a1c9791b208567b520ce50f829eed25d Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 04:15:55 +0000 Subject: [PATCH 3/7] Revert core port types to ToMlirType; remove e2e cmake test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TypeAliasType on core module ports caused CIRCT's export-verilog to emit 'logic TypedefName' for sv.logic declarations, which is invalid SystemVerilog (caught by the ecc_mem interface test). Revert DeclareCorePorts to use ToMlirType for core module ports. TypeAliasType is only needed in EmitEsiWrapper where ESI reads the types for C++ header generation — the core SV path must use anonymous struct types to avoid the 'logic' prefix issue. Remove the end-to-end cmake-script test (typealias_test.k and the file(WRITE)/cmake -P infrastructure) since: - The C++ TypeAliasTest in internal_tests.cpp covers the mechanism - The ecc_mem interface test covers SV integration with named types - The cmake -P pattern was not used elsewhere in the repo --- compiler/cpp/verilog.cpp | 6 +-- test/compiler/CMakeLists.txt | 68 +--------------------------------- test/compiler/typealias_test.k | 52 -------------------------- 3 files changed, 4 insertions(+), 122 deletions(-) delete mode 100644 test/compiler/typealias_test.k diff --git a/compiler/cpp/verilog.cpp b/compiler/cpp/verilog.cpp index d60d7331..c47b3c80 100644 --- a/compiler/cpp/verilog.cpp +++ b/compiler/cpp/verilog.cpp @@ -6562,7 +6562,7 @@ class VerilogCompiler coreModule.AddPort(port._name, port._input ? circt::hw::ModulePort::Direction::Input : circt::hw::ModulePort::Direction::Output, - ToMlirTypeAliased(port._type, false, coreModule), port._type, + ToMlirType(port._type), port._type, port._portSemantics, port._channelSemantics, port._channelName, port._fieldName); } @@ -6630,7 +6630,7 @@ class VerilogCompiler const Type *type = functionNode->GetParameterType(i); coreModule.AddPort(prefix + "_" + functionNode->GetParameterName(i) + "_out", circt::hw::ModulePort::Direction::Output, - ToMlirTypeAliased(type, false, coreModule), type, + ToMlirType(type), type, EsiPortSemantics::Payload, EsiChannelSemantics::FromGeneratedHw, EsiChannelName::Arguments, functionNode->GetParameterName(i)); } @@ -6654,7 +6654,7 @@ class VerilogCompiler { const Type *type = functionNode->GetReturnType(); coreModule.AddPort(prefix + "_result_in", circt::hw::ModulePort::Direction::Input, - ToMlirTypeAliased(type, false, coreModule), type, + ToMlirType(type), type, EsiPortSemantics::Payload, EsiChannelSemantics::ToGeneratedHw, EsiChannelName::Results); } diff --git a/test/compiler/CMakeLists.txt b/test/compiler/CMakeLists.txt index 4b171452..0b4e5a7d 100644 --- a/test/compiler/CMakeLists.txt +++ b/test/compiler/CMakeLists.txt @@ -40,77 +40,11 @@ add_test( # Phoney target to ensure compiler test dependencies have been built add_custom_target(compiler_tests DEPENDS compiler_internal_test) -# End-to-end test: compile a .k file with named types and verify the CIRCT IR -# output contains hw.typedecl and hw.typealias entries. -# Uses add_kanagawa() to compile, then cmake -P to grep the MLIR output. -set(_typealias_outdir "${CMAKE_CURRENT_BINARY_DIR}/typealias_test_output") - -add_kanagawa(typealias_test_hls - SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/typealias_test.k" - OPTIONS --backend=sv --write-circt-ir --import-dir=${CMAKE_SOURCE_DIR}/library - OUTPUT_DIR "${_typealias_outdir}" - CLEAN_OUTPUT_DIR -) - -# CMake script that checks the generated MLIR file for expected patterns -file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/check_typealias.cmake" [=[ -# Find the .mlir file in the output directory -file(GLOB MLIR_FILES "${OUTDIR}/*.mlir") -list(SORT MLIR_FILES) -list(LENGTH MLIR_FILES NUM_MLIR) -if(NUM_MLIR EQUAL 0) - message(FATAL_ERROR "No .mlir file found in ${OUTDIR}") -endif() -if(NOT NUM_MLIR EQUAL 1) - string(JOIN "\n " MLIR_FILE_LIST ${MLIR_FILES}) - message(FATAL_ERROR - "Expected exactly one .mlir file in ${OUTDIR}, found ${NUM_MLIR}:\n ${MLIR_FILE_LIST}") -endif() -list(GET MLIR_FILES 0 MLIR_FILE) -file(READ "${MLIR_FILE}" MLIR_CONTENT) -message(STATUS "Checking ${MLIR_FILE} for type aliases...") - -macro(check_pattern DESC PATTERN) - string(REGEX MATCH "${PATTERN}" _match "${MLIR_CONTENT}") - if(_match) - message(STATUS " PASS: ${DESC}") - else() - message(FATAL_ERROR " FAIL: ${DESC} (pattern: ${PATTERN})") - endif() -endmacro() - -check_pattern("TypeScope contains Point typedecl" "hw[.]typedecl @Point") -check_pattern("TypeScope contains Line typedecl" "hw[.]typedecl @Line") -check_pattern("TypeScope contains Color typedecl" "hw[.]typedecl @Color") -check_pattern("Core port uses Point type alias" "typealias<@CoreModuleTypeScope::@Point") -check_pattern("Core port uses Line type alias" "typealias<@CoreModuleTypeScope::@Line") -check_pattern("EsiWrapper container exists" "EsiWrapper") -check_pattern("ESI channel uses Color type alias" "typealias<@CoreModuleTypeScope::@Color") -message(STATUS "All type alias checks passed.") -]=]) - -add_test( - NAME compiler.typealias - COMMAND ${CMAKE_COMMAND} - -DOUTDIR=${_typealias_outdir} - -P "${CMAKE_CURRENT_BINARY_DIR}/check_typealias.cmake" -) -# The .k must be compiled before the test can run -set_tests_properties(compiler.typealias PROPERTIES - FIXTURES_REQUIRED typealias_test_hls_fixture -) -# Register the HLS compilation as a fixture-setup test -add_test( - NAME compiler.typealias_setup - COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target typealias_test_hls -) -set_tests_properties(compiler.typealias_setup PROPERTIES FIXTURES_SETUP typealias_test_hls_fixture) - # Add a target to run all compiler tests add_custom_target(run_compiler_tests COMMAND ${CMAKE_CTEST_COMMAND} --test-dir ${CMAKE_CURRENT_BINARY_DIR} -R "^compiler\\." --output-on-failure --verbose WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} - DEPENDS compiler_internal_test typealias_test_hls + DEPENDS compiler_internal_test COMMENT "Run all compiler internal tests" USES_TERMINAL ) diff --git a/test/compiler/typealias_test.k b/test/compiler/typealias_test.k deleted file mode 100644 index 7af4c10b..00000000 --- a/test/compiler/typealias_test.k +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -// Test that named struct and enum types are emitted as hw.typealias -// in the CIRCT IR output, both on core module ports and ESI wrapper ports. - -struct Point -{ - uint16 x; - uint16 y; -} - -struct Line -{ - Point start; - Point end; -} - -enum Color : uint8 -{ - Red, - Green, - Blue -} - -class TypeAliasTestModule -{ -public: - // Exported function with a named struct parameter and return - Point TranslatePoint(Point p, uint16 dx, uint16 dy) - { - Point result = { cast(p.x + dx), cast(p.y + dy) }; - return result; - } - - // Exported function with nested named struct parameter - Line TranslateLine(Line l, uint16 dx, uint16 dy) - { - Point s = { cast(l.start.x + dx), cast(l.start.y + dy) }; - Point e = { cast(l.end.x + dx), cast(l.end.y + dy) }; - Line result = { s, e }; - return result; - } - - // Exported function with enum type - uint8 GetColorValue(Color c) - { - return cast(c); - } -} - -export TypeAliasTestModule; From 48595565bd9ee23004d98730e1aeee01000a7981 Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 04:23:58 +0000 Subject: [PATCH 4/7] Address second round of review comments - Fix signedness mismatch: ToMlirTypeAliased now only substitutes aliases when signedness=false, since aliases are registered with default unsigned lowering. Signed requests fall through to the normal type lowering path. - Validate layout on name collision: when a different Type* shares the same name as an already-registered type, compare the underlying MLIR type. Throw a clear diagnostic if layouts differ instead of silently reusing an incompatible alias. - Eliminate code duplication: extract shared struct/union/array/leaf lowering into ToMlirTypeImpl() with an injectable recursion callback. Both ToMlirType and ToMlirTypeAliased delegate to it, eliminating the risk of the two implementations drifting apart. --- compiler/cpp/circt_util.cpp | 98 ++++++++++++++----------------------- 1 file changed, 37 insertions(+), 61 deletions(-) diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index 57b5a7de..611d0a54 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -497,7 +497,13 @@ circt::hw::ArrayType GetPackedArrayTypeParameterizedSize(const mlir::Type &eleme circt::seq::ClockType GetClockType() { return circt::seq::ClockType::get(g_compiler->GetMlirContext()); } -mlir::Type ToMlirType(const Type *typeIn, bool signedness) +// Internal helper: converts a Kanagawa Type to an MLIR type, using the provided +// 'recurse' callback for recursive member/element type conversion. +// This avoids duplicating struct/union/array lowering logic between +// ToMlirType and ToMlirTypeAliased. +using TypeRecurseFn = std::function; + +static mlir::Type ToMlirTypeImpl(const Type *typeIn, bool signedness, const TypeRecurseFn &recurse) { const BoolType *boolType = dynamic_cast(typeIn); const ArrayType *arrayType = dynamic_cast(typeIn); @@ -511,7 +517,7 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) } else if (arrayType) { - return GetPackedArrayType(ToMlirType(arrayType->_elementType, signedness), arrayType->_arraySize); + return GetPackedArrayType(recurse(arrayType->_elementType, signedness), arrayType->_arraySize); } else if (floatType) { @@ -523,17 +529,11 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) { llvm::SmallVector fields; - const auto addField = [&](const std::string &name, const Type *const type) - { - fields.push_back( - circt::hw::StructType::FieldInfo{StringToStringAttr(name), ToMlirType(type, signedness)}); - }; - for (const StructUnionType::EntryType &member : structUnionType->_members) { const Type *const memberType = member.second->GetDeclaredType(); - const std::string memberName = member.first; - addField(memberName, memberType); + fields.push_back( + circt::hw::StructType::FieldInfo{StringToStringAttr(member.first), recurse(memberType, signedness)}); } std::reverse(fields.begin(), fields.end()); @@ -543,17 +543,11 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) { llvm::SmallVector fields; - const auto addField = [&](const std::string &name, const Type *const type) - { - fields.push_back( - circt::hw::UnionType::FieldInfo{StringToStringAttr(name), ToMlirType(type, signedness)}); - }; - for (const StructUnionType::EntryType &member : structUnionType->_members) { const Type *const memberType = member.second->GetDeclaredType(); - const std::string memberName = member.first; - addField(memberName, memberType); + fields.push_back( + circt::hw::UnionType::FieldInfo{StringToStringAttr(member.first), recurse(memberType, signedness)}); } std::reverse(fields.begin(), fields.end()); @@ -579,55 +573,29 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) } } -mlir::Type ToMlirTypeAliased(const Type *typeIn, bool signedness, ModuleDeclarationHelper &helper) +mlir::Type ToMlirType(const Type *typeIn, bool signedness) { - // Check if this type has a registered alias - auto alias = helper.GetTypeAlias(typeIn); - if (alias) - { - return *alias; - } + return ToMlirTypeImpl(typeIn, signedness, [](const Type *t, bool s) { return ToMlirType(t, s); }); +} - // For struct types, recursively use aliased versions for member types - const StructUnionType *structUnionType = dynamic_cast(typeIn); - if (structUnionType) +mlir::Type ToMlirTypeAliased(const Type *typeIn, bool signedness, ModuleDeclarationHelper &helper) +{ + // Check if this type has a registered alias. + // Only substitute when signedness matches the alias registration (signedness=false), + // because aliases are created with default unsigned lowering and substituting them + // for a signed request would silently change the emitted MLIR type. + if (!signedness) { - if (structUnionType->_type == ContainerType::Struct) + auto alias = helper.GetTypeAlias(typeIn); + if (alias) { - llvm::SmallVector fields; - for (const StructUnionType::EntryType &member : structUnionType->_members) - { - const Type *const memberType = member.second->GetDeclaredType(); - fields.push_back(circt::hw::StructType::FieldInfo{StringToStringAttr(member.first), - ToMlirTypeAliased(memberType, signedness, helper)}); - } - std::reverse(fields.begin(), fields.end()); - return circt::hw::StructType::get(g_compiler->GetMlirContext(), fields); + return *alias; } - else - { - llvm::SmallVector fields; - for (const StructUnionType::EntryType &member : structUnionType->_members) - { - const Type *const memberType = member.second->GetDeclaredType(); - fields.push_back(circt::hw::UnionType::FieldInfo{StringToStringAttr(member.first), - ToMlirTypeAliased(memberType, signedness, helper)}); - } - std::reverse(fields.begin(), fields.end()); - return circt::hw::UnionType::get(g_compiler->GetMlirContext(), fields); - } - } - - // For array types, recursively use aliased versions for element type - const ArrayType *arrayType = dynamic_cast(typeIn); - if (arrayType) - { - return GetPackedArrayType(ToMlirTypeAliased(arrayType->_elementType, signedness, helper), - arrayType->_arraySize); } - // For all other types, fall back to the non-aliased version - return ToMlirType(typeIn, signedness); + // Delegate to the shared implementation with alias-aware recursion + return ToMlirTypeImpl(typeIn, signedness, + [&helper](const Type *t, bool s) { return ToMlirTypeAliased(t, s, helper); }); } // Used to avoid symbol name conflicts for elements like container ports @@ -1812,10 +1780,18 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) } // Check if a different Type* with the same name was already registered. - // Reuse the existing alias to prevent duplicate hw.typedecl symbols. + // Reuse the existing alias to prevent duplicate hw.typedecl symbols, + // but verify the underlying layout matches to catch conflicting definitions. auto nameIt = _typeAliasByName.find(typeName); if (nameIt != _typeAliasByName.end()) { + circt::hw::TypeAliasType existingAlias = llvm::cast(nameIt->second); + mlir::Type newMlirType = ToMlirTypeAliased(kanagawaType, false, *this); + if (existingAlias.getInnerType() != newMlirType) + { + throw std::runtime_error("Conflicting named type definitions for '" + typeName + + "': existing alias has a different underlying layout"); + } _typeAliasCache[kanagawaType] = nameIt->second; return; } From 68f4628e778c2dd562f3049bee7a1090b02af32b Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 04:33:11 +0000 Subject: [PATCH 5/7] Fix signedness bug; add test coverage for ESI wrapper path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous signedness guard in ToMlirTypeAliased skipped the alias lookup when signedness=true, but EmitEsiWrapper (the only consumer) always passes signedness=true. This meant ESI wrapper ports never actually got type aliases — the feature was silently broken. Fix: register aliases with signedness=true (matching the ESI consumer) and remove the signedness guard so aliases are returned unconditionally. Strengthen TypeAliasTest to verify: - Aliases are returned for BOTH signedness=true and signedness=false (catches the regression that broke the ESI wrapper path) - The alias inner type uses signed integers (ui8 not i8), matching what EmitEsiWrapper expects - Both signedness variants return the same alias object --- compiler/cpp/circt_util.cpp | 24 ++++++++++-------------- compiler/cpp/internal_tests.cpp | 27 +++++++++++++++++++-------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index 611d0a54..a6c2054c 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -580,17 +580,11 @@ mlir::Type ToMlirType(const Type *typeIn, bool signedness) mlir::Type ToMlirTypeAliased(const Type *typeIn, bool signedness, ModuleDeclarationHelper &helper) { - // Check if this type has a registered alias. - // Only substitute when signedness matches the alias registration (signedness=false), - // because aliases are created with default unsigned lowering and substituting them - // for a signed request would silently change the emitted MLIR type. - if (!signedness) - { - auto alias = helper.GetTypeAlias(typeIn); - if (alias) - { - return *alias; - } + // Check if this type has a registered alias + auto alias = helper.GetTypeAlias(typeIn); + if (alias) + { + return *alias; } // Delegate to the shared implementation with alias-aware recursion @@ -1786,7 +1780,7 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) if (nameIt != _typeAliasByName.end()) { circt::hw::TypeAliasType existingAlias = llvm::cast(nameIt->second); - mlir::Type newMlirType = ToMlirTypeAliased(kanagawaType, false, *this); + mlir::Type newMlirType = ToMlirTypeAliased(kanagawaType, true, *this); if (existingAlias.getInnerType() != newMlirType) { throw std::runtime_error("Conflicting named type definitions for '" + typeName + @@ -1806,8 +1800,10 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) } } - // Build the MLIR type using the alias-aware conversion so inner named types use aliases - mlir::Type mlirType = ToMlirTypeAliased(kanagawaType, false, *this); + // Build the MLIR type using the alias-aware conversion so inner named types use aliases. + // Use signedness=true to match the ESI wrapper consumer (the only caller of ToMlirTypeAliased), + // which passes signedness=true to produce signed/unsigned integer types. + mlir::Type mlirType = ToMlirTypeAliased(kanagawaType, true, *this); // Create the TypedeclOp in the TypeScope block { diff --git a/compiler/cpp/internal_tests.cpp b/compiler/cpp/internal_tests.cpp index 502f3e8b..b636bb4a 100644 --- a/compiler/cpp/internal_tests.cpp +++ b/compiler/cpp/internal_tests.cpp @@ -2533,8 +2533,7 @@ void TypeAliasTest() ModuleDeclarationHelper helper(writer, "TestModule", GetUnknownLocation(), "TestDesign", &mlirModule); helper.AddTypedefs("TestTypeScope"); - // Before registration, ToMlirTypeAliased should return the same as ToMlirType - mlir::Type plainType = ToMlirType(&testEnum); + // Before registration, ToMlirTypeAliased should not return an alias mlir::Type aliasedBefore = ToMlirTypeAliased(&testEnum, false, helper); TestAssert(!llvm::isa(aliasedBefore)); @@ -2546,14 +2545,26 @@ void TypeAliasTest() TestAssert(alias.has_value()); TestAssert(llvm::isa(*alias)); - // The alias should wrap the same inner type + // The alias should have the correct name circt::hw::TypeAliasType aliasType = llvm::cast(*alias); - TestAssert(aliasType.getInnerType() == plainType); TestAssertEqual(std::string("TestColor"), aliasType.getRef().getLeafReference().str()); - // ToMlirTypeAliased should now return the alias - mlir::Type aliasedAfter = ToMlirTypeAliased(&testEnum, false, helper); - TestAssert(llvm::isa(aliasedAfter)); + // The alias inner type should use signed integer types (signedness=true), + // matching the ESI wrapper which is the consumer of type aliases + mlir::Type expectedInnerType = ToMlirType(&testEnum, true); + TestAssert(aliasType.getInnerType() == expectedInnerType); + + // ToMlirTypeAliased should return the alias for BOTH signedness values. + // This is critical: EmitEsiWrapper calls with signedness=true, + // so aliases must be returned regardless of the signedness flag. + mlir::Type aliasedSignedFalse = ToMlirTypeAliased(&testEnum, false, helper); + TestAssert(llvm::isa(aliasedSignedFalse)); + + mlir::Type aliasedSignedTrue = ToMlirTypeAliased(&testEnum, true, helper); + TestAssert(llvm::isa(aliasedSignedTrue)); + + // Both should return the same alias + TestAssert(aliasedSignedFalse == aliasedSignedTrue); // Registering the same type twice should be a no-op helper.RegisterNamedType(&testEnum); @@ -2566,7 +2577,7 @@ void TypeAliasTest() auto noAlias = helper.GetTypeAlias(u16Type); TestAssert(!noAlias.has_value()); - // Verify the MLIR module can be printed (basic sanity) + // Verify the MLIR module contains the typedecl std::string mlirStr; llvm::raw_string_ostream os(mlirStr); mlirModule.print(os); From 5d0ef63a5254f0f9e8fd757345050a18761697be Mon Sep 17 00:00:00 2001 From: John Demme Date: Wed, 15 Apr 2026 05:31:04 +0000 Subject: [PATCH 6/7] Fix crashes on non-hardware types in RegisterNamedType MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues caused ICEs and invalid SV when compiling real test cases: 1. Structs with non-MLIR-convertible members (e.g., callback function types) caused an assert(false) in ToMlirTypeImpl when RegisterNamedType tried to build the MLIR type. Fix: check that all struct members are MLIR-convertible types before attempting registration. 2. Types with template-instantiation names containing special characters (e.g., '@type@newtype::newtype<|$uint@32|>') produced invalid SV identifiers when emitted as hw.typedecl symbols. Fix: skip types whose names aren't valid identifiers (alphanumeric + underscore). 3. Removed the recursive member-type registration that was added as a 'safety net' — it's unnecessary since _exportedTypes is already topologically sorted, and it triggered the crashes above. Verified: all 87 compiler + interface + syntax tests pass. --- compiler/cpp/circt_util.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index a6c2054c..3a33eeb4 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -1767,12 +1767,20 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) typeName = enumType->GetName(); } - // Only register types with a non-empty name + // Only register types with a non-empty name that is a valid identifier + // (no special characters from template instantiations, etc.) if (typeName.empty()) { return; } + const bool isValidIdentifier = std::all_of(typeName.begin(), typeName.end(), + [](char c) { return std::isalnum(c) || c == '_'; }); + if (!isValidIdentifier) + { + return; + } + // Check if a different Type* with the same name was already registered. // Reuse the existing alias to prevent duplicate hw.typedecl symbols, // but verify the underlying layout matches to catch conflicting definitions. @@ -1790,13 +1798,28 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) return; } - // Recursively register member types first (for structs/unions) + // Note: we do NOT recursively register member types here. + // The caller (DeclareCore) iterates _exportedTypes which is already + // topologically sorted by SortExportedTypes(), so member types are + // registered before their containing structs. Recursing here would + // crash on non-hardware member types (ClassType, ReferenceType, etc.) + // that can't be converted to MLIR. + + // Verify the type can be converted to MLIR before attempting registration. + // Some exported types (e.g., structs containing callback function members) + // have members that ToMlirType cannot handle. Skip those silently. if (structUnionType) { for (const StructUnionType::EntryType &member : structUnionType->_members) { const Type *memberType = member.second->GetDeclaredType(); - RegisterNamedType(memberType); + if (!dynamic_cast(memberType) && !dynamic_cast(memberType) && + !dynamic_cast(memberType) && !dynamic_cast(memberType) && + !dynamic_cast(memberType)) + { + // Member type is not MLIR-convertible — skip this type + return; + } } } From 90f360244f497b525ee3c76645962d36466df025 Mon Sep 17 00:00:00 2001 From: Sam Dietrich Date: Mon, 27 Apr 2026 14:38:19 -0700 Subject: [PATCH 7/7] fixup typeName --- compiler/cpp/circt_util.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/cpp/circt_util.cpp b/compiler/cpp/circt_util.cpp index 3a33eeb4..0a3769be 100644 --- a/compiler/cpp/circt_util.cpp +++ b/compiler/cpp/circt_util.cpp @@ -1767,19 +1767,15 @@ void ModuleDeclarationHelper::RegisterNamedType(const Type *kanagawaType) typeName = enumType->GetName(); } - // Only register types with a non-empty name that is a valid identifier - // (no special characters from template instantiations, etc.) + // Only register types with a non-empty name. if (typeName.empty()) { return; } - const bool isValidIdentifier = std::all_of(typeName.begin(), typeName.end(), - [](char c) { return std::isalnum(c) || c == '_'; }); - if (!isValidIdentifier) - { - return; - } + // Kanagawa type names typically contain '.' from namespacing. + // Normalize identifier the same way the SV backend does. + typeName = FixupString(typeName); // Check if a different Type* with the same name was already registered. // Reuse the existing alias to prevent duplicate hw.typedecl symbols,