[Debug] [1/13] Add EnumOp and ValueOp to the Debug dialect

[fkhaidari]

EnumOp interprets an integer value as a tag-only enumeration, carrying the name -> tag variant map in its attributes and producing an enum-typed debug value (EnumType). It behaves like a cast: the result embeds directly in dbg.variable, dbg.struct, and dbg.array like any other value, so enum tracking needs no separate definition op, scope, or wrapper linkage. The op is Pure since it has no side effects and its result is fully determined by its operands and attributes, so unused casts fold away under DCE while identical-content enums deduplicate across modules by content key at emission.

ValueOp annotates an SSA value with optional source-language type metadata (type name and constructor parameters) and a dedicated location. Its result is a metadata wrapper accepted by dbg.variable, dbg.struct, and dbg.array, keeping those ops orthogonal to source-language type information; an enum value wraps in a dbg.enum first, so no enum linkage is carried on the wrapper itself.

Based on @rameloni's work

[fkhaidari]

This pr is a part of the split version of #10410. More: https://discourse.llvm.org/t/uhdi-unified-hardware-debug-info-structured-debug-info-export-for-chisel-firrtl/90973

[uenoku]

I think this is simply CSE and you can add Pure trait to the operation to allow CSE.
Also please don't introduce a canonicalizer pattern that walks all most entire IR.

[fkhaidari]

Thanks for catching this. Dropped the canonicalizer and hasCanonicalizer. You're right that the per-op block scan was the wrong tool; it had a TODO(perf): O(N^2) on it already.

I kept the op non-Pure. This is the base PR of a series, and the later passes that consume dbg.enumdef rely on it surviving even with no SSA users: in multi-module designs a shared enum may be declared in its owning module but referenced from child modules only, so the owning dbg.enumdef can have zero users yet still must reach the emitter. Pure would let DCE delete it. Those consumers also dedupe enumdefs by key when they build their tables, so duplicates left in the IR are harmless and the canonicalizer wasn't buying us anything.

[programmerjake]

should this support enumerations where the variants have data? e.g. FIRRTL:

{|A, B: UInt<8>, C: {a: UInt<1>[3], b: Clock}|}

[fkhaidari]

Good question. dbg.enumdef here models tag-only (C-style) enums: a variant is a name bound to an integer tag, and variantsMap has no slot for a per-variant payload type. So data-carrying variants like your {|A, B: UInt<8>, ...|} aren't covered, and I clarified that scope in the op description.

My feeling is those are different enough that they'd want their own op rather than overloading this one. A data-carrying variant is really a tagged union: the tag selects a variant, and each variant has its own payload layout to render, which is closer to a discriminated dbg.struct/dbg.array than to a name-to-tag map. I'd rather keep dbg.enumdef to the simple tag-only case and add a separate op for sum types if/when there's a producer and a consumer that need them.

[programmerjake]

the new section about enums looks good, I'll leave the rest for others to review

[uenoku]

I think this condition is redundant because all_of returns true for empty.

[fkhaidari]

Thanks! Addressed

[uenoku]

Consider using SmallDenseSet.

[fkhaidari]

Addressed

[uenoku]

Please don't walk entire IR in verifier. From that view point I'm also not sure the representation of this operation composes well. This also means we won't be able to inline modules with this operation. Is there a problem with storing this as (discardable) attributes or FusedLoc's metadata attribute on module?

[fkhaidari]

Thanks for the catch! I agree with you. Dropped ModuleInfoOp. Following commits will replace it with a discardable attribute

[fabianschuiki]

Hey @fkhaidari, sorry for taking so long to respond. Thanks a lot for pushing on this and building out the debug info capabilities 🥳.

I'm wondering whether we really need to capture enum definitions as an SSA value via dbg.enumdef in the IR at all. I suspect that what you are looking for is a way to take an integer value in the IR, say "this is an enum with these variants", and track it as a debug value in a variable, or nested in a struct or array; is that correct?

If that's the case, I think you could extend the pattern of dbg.array (create an array value) and dbg.struct (create a struct value) by adding a new dbg.enum (create an enum value) op with a corresponding EnumType:

%enumValue = dbg.enum %intValue, "MyEnumName", {...}

This behaves a bit like a cast, interpreting an integer value as an enum, and gives you an enum-typed SSA value. That value you can then embed in dbg.array, dbg.struct, and dbg.variable just like any other value, and you don't need to deal with scopes or subfields with additional ops.

What do you think?

[circt-bot]

Results of circt-tests run for a6d00a2 compared to results for 937f4b9: no change to test results.

[fkhaidari]

I think you could extend the pattern of dbg.array (create an array value) and dbg.struct (create a struct value) by adding a new dbg.enum (create an enum value) op with a corresponding EnumType

Hi @fabianschuiki, thank you for your comment!

You read the intent exactly right, and I agree the cast-style dbg.enum + EnumType is the cleaner fit. I took your suggestion and reworked commit around it. dbg.enumdef is gone entirely.

One nice consequence: this also settles the earlier non-Pure thread with @uenoku. Each use site now materialises its own enum value, so it always has a user and survives DCE on its own.

[uenoku]

I think this is not used anymore?

[fkhaidari]

You are right, thank you! Addressed

[fabianschuiki]

Cool! Do you still need SubFieldOp and the typeName and params on VariableOp? The subfield op feels like the same pattern as your previous enum op; could we just attach subfield info to the dbg.struct op since it actually creates a struct from a bunch fields? We could attach any metadata there.

[circt-bot]

Results of circt-tests run for 5a0fc28 compared to results for 937f4b9: no change to test results.

[fkhaidari]

@fabianschuiki, the typeName/params on VariableOp are still needed: they carry source language type info (class name, constructor params) that isn't recoverable from the IR after lowering, and scalar variables have no dbg.struct to hang them on.

SubFieldOp has the same attributes but a different role: dbg.variable has no result and is treated as a root entry of the module scope, while a subfield produces a value that feeds into dbg.struct/dbg.array.

Moving the metadata onto the aggregate op would work, but the wrapper buys a few things for free: only annotated leaves pay for it (most fields carry no metadata, so a per-field attribute array would be mostly placeholders), each leaf keeps its own source location while the struct has a single loc, and one mechanism covers both dbg.struct and dbg.array. It also keeps the metadata tied to the value through the use-def chain rather than to an operand position, so it stays consistent if a pass ever rebuilds the aggregate

[fabianschuiki]

Oh I see, that makes perfect sense. We haven't really had a good way to attach more detailed metadata to values in the past. This makes me wonder: instead of adding typename/params to variable and subfield op, and I guess later also other ops, could we instead make the two things orthogonal? For example, keep dbg.variable simple as it is, but add a dbg.value op (instead of subfield op) which simply annotates an SSA value with additional information like that typename, parameters, a dedicated location, etc. We might be able to use that for other things in the future, too. That would allow us to keep the ops orthogonal, and tuck value metadata into dbg.value while tracking variables as dbg.variable. WDYT?

[fkhaidari]

@fabianschuiki, I like this! Implemented:

%0 = dbg.value %wire typeName "UInt<8>" params [{name = "width", value = "8"}] : i8
dbg.variable "counter", %0 : !dbg.value

dbg.variable is back to its original form, and dbg.subfield is gone

[fabianschuiki]

Very nice! LGTM, thanks a lot 🥳

[circt-bot]

Results of circt-tests run for d25bd35 compared to results for 937f4b9:

sv-tests

Changes in emitted diagnostics:

• +55 total change
• +22 error: unsupported construct in ClassType members: ConstraintBlock
• +7 error: identifier 'analysis_txn' used before its declaration
• +6 error: no member named 'sprint' in 'm_uvm_printer_knobs'
• +3 error: parentheses are required when invoking function 'get_status'
• +3 error: value of type 'T' (aka 'uvm_sequence_base') cannot be assigned to type 'simple_seq'
• +3 error: virtual method 'sample' is 'public' but declared 'protected' in superclass method
• +2 error: time scale declaration must come before all other items in scope
• -1 error: unknown module 'BUFGCE'
• +1 error: unknown macro or compiler directive '`uvm_sequence_utils'
• +1 error: unknown macro or compiler directive '`uvm_sequencer_utils'
• +1 error: unknown module 'pss'
• +1 error: unknown module 'pss_wrapper'
• +1 error: unknown module 'spi_clgen'
• +1 error: unknown module 'spi_shift'
• +1 error: unsupported format specifier `%c`
• +1 error: use of undeclared identifier 'CONTROL'
• +1 error: use of undeclared identifier 'count'
• +1 error: value of type 'T' (aka 'uvm_sequencer') cannot be assigned to type 'simple_sequencer'