Emit hw.typealias for named types in CIRCT IR output#121
Draft
teqdruid wants to merge 8 commits into
Draft
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds CIRCT hw.typedecl/hw.typealias emission for named Kanagawa types (structs/unions/enums) and updates IR type lowering so ESI-visible ports reference these aliases, ensuring generated ESI C++ headers preserve meaningful type names.
Changes:
- Introduces
RegisterNamedType()/GetTypeAlias()caching andToMlirTypeAliased()for alias-aware recursive type lowering. - Registers exported named types in the core TypeScope before port declaration; uses alias-aware lowering for core ports and ESI wrapper payloads.
- Adds unit + end-to-end tests validating
hw.typedeclcreation andtypealias<...>usage in written CIRCT IR.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/compiler/typealias_test.k | Adds an end-to-end .k source exercising named structs/enums on exported APIs. |
| test/compiler/CMakeLists.txt | Adds an end-to-end CTest that builds IR and checks for hw.typedecl + hw.typealias patterns. |
| compiler/cpp/verilog.cpp | Registers named types in the TypeScope and switches relevant ports/payloads to alias-aware type lowering. |
| compiler/cpp/internal_tests.cpp | Adds a C++ unit test covering RegisterNamedType() + ToMlirTypeAliased() behavior. |
| compiler/cpp/circt_util.h | Declares ToMlirTypeAliased() and new ModuleDeclarationHelper alias APIs/state. |
| compiler/cpp/circt_util.cpp | Implements alias-aware type lowering + registration/caching of typedecls/typealiases. |
| .github/copilot-instructions.md | Adds repository guidance for AI-assisted changes (non-functional). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
- 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.
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
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
AI-assisted-by: Claude Opus 4.6