[debug][firrtl] Carry Chisel source-level types through the pipeline#10410
Open
fkhaidari wants to merge 5 commits into
Open
[debug][firrtl] Carry Chisel source-level types through the pipeline#10410fkhaidari wants to merge 5 commits into
fkhaidari wants to merge 5 commits into
Conversation
fkhaidari
force-pushed
the
fk-sc/debug-info
branch
from
75a087e to
b62473b
Compare
Author
|
Split the original commit into 5 focused commits for easier review. @darthscsi, @seldridge, @fabianschuiki, could you take a look? |
|
Results of circt-tests run for b62473b compared to results for 2f8e9c7: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
fkhaidari
force-pushed
the
fk-sc/debug-info
branch
from
b62473b to
dc3c770
Compare
EnumDefOp describes the integer->name variant map for an enumeration type. Its result (EnumDefType) is consumed by dbg.variable and dbg.subfield via an optional operand, letting waveform viewers render raw integer signals by name. A canonicalizer deduplicates EnumDefOps with identical (enumTypeName, fqn, variantsMap, scope) within a block using backward-dominance scan. The op is intentionally not Pure so DCE does not remove enumdefs whose IDs are referenced by downstream metadata outside the IR. SubFieldOp tags a single aggregate leaf for debug info, carrying the same optional typeName / params / enumDef metadata as dbg.variable. Its result (SubFieldType) must be consumed by dbg.struct or dbg.array; the verifier tolerates zero-user ops during incremental construction. ModuleInfoOp attaches source-language type info (typeName, params) to a module region. The verifier enforces at most one per region. VariableOp gains optional typeName and params attributes and an optional enumDef operand to mirror the per-leaf metadata introduced by SubFieldOp. The existing backward-compatible build helper retains the old (name, value, scope) signature.
…ugInfo Adds DIEnumDefId (uint32_t), DIEnumValMap, DIEnumDefMap, and DISourceLang to DebugInfo.h to carry enum variant tables and source-language type annotations through the analysis. DIModule gains three new fields: enumDefinitions (the id->variants table), enumDefIds (op->id map for O(1) lookup from a Value), and sourceLangType (populated from dbg.moduleinfo). DIVariable gains enumDef (the raw SSA value), enumDefRef (the module-scoped integer id for serialisation), and sourceLangType (from dbg.variable typeName/params attributes). The module walk is changed to PreOrder so every dbg.enumdef is assigned an id before any dbg.variable that references its result. Duplicate enumdefs (same enumTypeName + fqn + variantsMap) reuse the first-seen id instead of creating a second entry. Inline dbg.scope ops now set hasInstances = true so the fallback HW-instance path is not triggered spuriously.
…metadata
Introduces a two-phase lowering for the debug intrinsics.
Phase 1 -- liftDebugIntrinsics() (new pre-pass, single module walk):
- circt_debug_enumdef intrinsics are converted to dbg.enumdef ops at module
scope, deduplicated by fqn. A variant-map mismatch on fqn collision emits a
warning and keeps the first definition.
- circt_debug_subfield intrinsics are collected into a DebugLeafList of
DictionaryAttrs (keys: parent, name, typeName?, params?, enumFqn?) and
erased. The parent key is the enclosing var's name and is the sole
var<->leaf linkage; name is the full dotted/indexed display path.
Both intrinsic kinds are erased after collection.
Phase 2 -- IntrinsicLowerings::lower() (existing path extended):
CirctDebugVarConverter reads DebugLeafList and enumDefByFqn through the new
IntrinsicConvertContext, emitting dbg.variable ops with typeName, params, and
enumDef operands populated from the source intrinsics.
IntrinsicConverter gains a convertWithContext() virtual that defaults to the
existing context-free convert(); checkAndConvert() now accepts
IntrinsicConvertContext and routes through convertWithContext(). A new
addConverter<T>(name, args...) overload supports converters with constructor
arguments. IntrinsicConvertContext is stack-allocated per lower() call and
never stored on the shared IntrinsicLowerings object, keeping parallel pass
invocations race-free.
…intrinsics When circt_debug_var intrinsics are present, MaterializeDebugInfo must not emit a second dbg.variable for the same signal; LowerIntrinsics will handle those variables. Build two skip-sets before the main walk: a DenseSet<Value> for single-operand vars (keyed by SSA value, not name, to avoid false positives across scopes) and a DenseSet<StringAttr> for zero-operand vars (memory-type). Any port, wire, reg, or node whose value or name appears in a skip-set is silently omitted from materialization. Adds an ordering guard: if dbg.variable ops already exist at pass entry, LowerIntrinsics ran before MaterializeDebugInfo. Rather than silently creating duplicates, emit a per-module warning and return early. The required pass order (MaterializeDebugInfo -> LowerIntrinsics) is now documented in the file header. Duplicate circt_debug_var intrinsics (same SSA value or name seen more than once in a module) emit per-op warnings to surface incorrect frontends.
…pper LowerCHIRRTL remaps operands that read CHIRRTL memory rdata ports to the equivalent values after memory lowering. Before this fix, dbg.variable and dbg.subfield ops referencing memory rdata were silently dropped: the base-class visitInvalidOp is a no-op, so neither the operands were remapped nor the ops forwarded to the FIRRTL visitor. Override visitInvalidOp to call visitUnhandledOp for any op belonging to the Debug dialect; this triggers the standard operand-remapping path. All other non-FIRRTL, non-CHIRRTL ops retain the original no-op behaviour.
fkhaidari
force-pushed
the
fk-sc/debug-info
branch
2 times, most recently
from
4ba5c85 to
befe397
Compare
Author
|
FYI: chipsalliance/chisel#5276 was merged to Chisel |
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
1 participant
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.
Hi! This commit carry Chisel source-level type information (type names, enum definitions, constructor parameters) through the FIRRTL -> MLIR pipeline so downstream consumers (Tywaves, HGDB) can recover the original Chisel types
Based on @rameloni's prior Tywaves/CIRCT debug-info work (#6983, #7246, chipsalliance/chisel#4224, chipsalliance/chisel#4015), but uses intrinsics instead of annotations. This PR pairs with the frontend intrinsics from chipsalliance/chisel#5276
The dbg dialect extensions (dbg.enumdef, dbg.subfield, dbg.moduleinfo) are language-agnostic and reusable by any frontend; the FIRRTL side adds handlers in firrtl-lower-intrinsics for the new circt_debug_{var,subfield,enumdef,moduleinfo} intrinsics that feed them
I am also working on a new debug output format (UHDI) -- it is intended to avoid changes to the current HGLDD format and to let existing HGL debug tools (Tywaves, HGDB, ChiselTrace, etc.) work without maintaining outdated forks. Work is still in progress (demo repo: https://github.com/fkhaidari/uhdi, CIRCT UHDI PR: fkhaidari#2).