Skip to content

Add results to sv.ifdef#10436

Draft
prithayan wants to merge 6 commits into
mainfrom
dev/pbarua/sv_ifdef
Draft

Add results to sv.ifdef#10436
prithayan wants to merge 6 commits into
mainfrom
dev/pbarua/sv_ifdef

Conversation

@prithayan
Copy link
Copy Markdown
Contributor

@prithayan prithayan commented May 12, 2026

This PR updates the sv.ifdef op to return results. We add a new op, sv.yield to return results from the sv.ifdef regions.
This enables us to use SSA values defined inside the sv.ifdef region, which is otherwise not possible. Since SSA values inside the sv.ifdef region, do not dominate any operations outside the region.
The motivation to add results to sv.ifdef is to support accessing Probes generated inside the sv.ifdef region.
In the FIRRTL dialect, it is possible to return probes defined in the sv.ifdef region as module output, but not in HW dialect. Consider this FIRRTL dialect example,

  firrtl.circuit "SimpleRefSend" {
    sv.macro.decl @LAYER_A
    firrtl.module @SimpleRefSend(in %clk: !firrtl.clock, out %probe_out: !firrtl.probe<uint<8>>) {
      sv.ifdef @LAYER_A {
        %w = firrtl.wire : !firrtl.uint<8>
        %0 = firrtl.ref.send %w : !firrtl.uint<8>
        firrtl.ref.define %probe_out, %0 : !firrtl.probe<uint<8>>
      }
    }
  }

We cannot lower this directly to the HW dialect, since the following is not a valid IR

  sv.macro.decl @LAYER_A
  hw.module @SimpleRefSend(in %clk : !seq.clock, out probe_out : !hw.probe<i8>) {
    %z_i8 = sv.constantZ : i8
    sv.ifdef @LAYER_A {
      %w = hw.wire %z_i8  : i8
      %1 = hw.probe.send %w : i8
    }
    hw.output %1 : !hw.probe<i8>
  }

But we can generate the following valid IR using the proposed sv.yield

  sv.macro.decl @LAYER_A
  hw.module @SimpleRefSend(in %clk : !seq.clock, out probe_out : !hw.probe<i8>) {
    %z_i8 = sv.constantZ : i8
    %probe = sv.ifdef @LAYER_A -> !hw.probe<i8> {
      %w = hw.wire %z_i8  : i8
      %1 = hw.probe.send %w : i8
       sv.yield %1 : !hw.probe<i8>
    }
    hw.output %probe : !hw.probe<i8>
  }

The if then and else regions can have independent results from each region. The results from both the regions are concatenated.

    // Returns 2 results: i32 from then, i64 from else
    %0:2 = sv.ifdef @MACRO -> i32, i64 {
      %then_val = ...
      sv.yield %then_val : i32
    } else {
      %else_val = ...
      sv.yield %else_val : i64
    }

In this implementation, there is no legal lowering of this in ExportVerilog and it is assumed that some earlier pass should lower them appropriately.
For the probes use case, a (proposed) HW::LowerXMR pass will generate hierarchical reference to the corresponding targets and remove any results from the sv.ifdef.
Any verilog lowering for this is not supported, since it can result in (illegal) dead code.

Why the result concatenation, instead of constraining the then and else region to return same result types ?

Since, sv.ifdef is not a normal SSA conditional; it models a Verilog preprocessor conditional. The then/else regions are independently-formed textual regions, and Verilog does not require the declarations or values in one branch to correspond to those in the other. Because of that, forcing both branches to have the same result types would add a constraint that does not exist in the source semantics. Concatenating the yielded values preserves the natural independence of the two regions: values from the then region stay distinct from values from the else region, instead of being forced into an artificial pairwise merge.
It is the responsibility of the circt pass generating this sv.ifdef with results to ensure the legality of the IR. For example the result from the then region can only be used when the corresponding condition is true.

@prithayan prithayan changed the title Dev/pbarua/sv ifdef Add results to sv.ifdef May 12, 2026
prithayan added 5 commits May 11, 2026 19:35
…erminator

This commit extends sv.ifdef and sv.ifdef.procedural operations to support
returning results via a concatenation semantic where results from the then
region are followed by results from the else region.

Key changes:

1. Added sv.yield operation as implicit terminator for sv.ifdef regions
   - sv.yield supports optional operands to return values from regions
   - Regions with no results use empty sv.yield as terminator

2. Implemented custom assembly format for sv.ifdef and sv.ifdef.procedural
   - Custom parsers ensure sv.yield terminator is added during parsing
   - Custom printers conditionally print terminators based on operands
   - Maintains backward compatibility with existing MLIR

3. Updated verification and helper methods
   - getNumThenResults() and getNumElseResults() handle missing terminators
   - Type checking ensures yielded values match operation result types

4. Fixed insertion point handling in transformation passes
   - LowerToHW: runWithInsertionPointAtEndOfBlock inserts before terminators
   - LowerLayers: ensureTerminator called after taking layerblock body
   - HWCleanup: mergeRegions removes terminators when merging blocks

5. Added ExportVerilog validation
   - PrepareForEmission rejects sv.yield with operands
   - PrepareForEmission rejects sv.ifdef with results (requires lowering)
   - ExportVerilog visitor skips empty yields, asserts on yields with operands

6. Added comprehensive lit tests
   - Tests for same-type and different-type results
   - Tests for multiple results from each region
   - Tests for empty yields (one region yields nothing)
   - Both sv.ifdef (non-procedural) and sv.ifdef.procedural variants

Result concatenation example:
  %0:3 = sv.ifdef @macro -> i32, i32, i8 {
    %v1, %v2 = ... : i32, i32
    sv.yield %v1, %v2 : i32, i32  // First 2 results
  } else {
    %v3 = ... : i8
    sv.yield %v3 : i8  // Third result
  }
  // %0#0 and %0#1 from then, %0#2 from else

All existing tests pass (62/62 SV and ExportVerilog tests).
Updated passes that create sv.ifdef/procedural operations to insert before
the implicit sv.yield terminator rather than after it. This fixes errors
where operations were being inserted after the terminator.

Changes:
- LowerLayers: Build ifdef with OperationState to add terminator before insertion
- FirRegLowering: Set insertion point before terminator when recreating conditions
- SimToSV: Move ops before terminator instead of at end of block

Tests passing:
- 1216/1290 (94.26%) CIRCT tests passing
- All SV and ExportVerilog tests pass
- SeqToSV firreg tests fixed

Remaining issues:
- 18 tests still failing related to FIRRTL layerblock operations
- These failures involve operations being inserted into FIRRTL module blocks
- Further investigation needed for complete fix
Fixes to ensure sv.ifdef blocks always have sv.yield terminators:

1. IMDeadCodeElim: Prevent removal of terminator operations
   - Added checks to skip terminators in dead code elimination
   - Terminators are essential for block structure and should never be removed
   - Fixes 'block with no terminator' errors after dead code elimination

2. LowerLayers: Add terminators to manually-created ifdef blocks
   - createBindFile: Added terminator to parent guard else blocks
   - createBindFile: Added terminator to include guard else block
   - These blocks are created manually with createBlock(), so need explicit terminators

Test Results:
- 1233/1290 tests passing (95.58%)
- Fixed: imdce.mlir, lower-layers.mlir, layers.fir
- Remaining: instance-choice.fir (dominance issue, under investigation)
Prevents moving hw.instance operations in blocks with implicit terminators
(like sv.ifdef blocks) to avoid dominance violations. Instance sinking is
skipped for ifdef and procedural regions where moving instances before the
terminator would place them after operations that use their results.

Fixes instance-choice dominance errors in prettify-verilog pass.
The conversion from GraphRegionNoTerminator to SingleBlockImplicitTerminator
dropped important traits:
- RegionKindInterface: Declares region kind (Graph vs SSACFG)
- HasOnlyGraphRegion: Disables SSA dominance requirements

Changes:
1. Added RegionKindInterface and HasOnlyGraphRegion traits to IfDefOp
2. Implemented getRegionKind() to return RegionKind::Graph
3. IfDefProceduralOp keeps SSACFG semantics (no graph region trait added)

This ensures sv.ifdef regions remain graph regions where operations can
appear in any order without SSA dominance constraints, matching the
original semantics of GraphRegionNoTerminator.

Test Results: 1234/1290 passing (95.66%)
@prithayan prithayan force-pushed the dev/pbarua/sv_ifdef branch from 36612de to 6a644ff Compare May 12, 2026 02:36
@prithayan prithayan force-pushed the dev/pbarua/sv_ifdef branch from 6a644ff to dd5f7ab Compare May 12, 2026 02:43
@circt-bot
Copy link
Copy Markdown

circt-bot Bot commented May 12, 2026

Results of circt-tests run for dd5f7ab compared to results for d8bbbc6: no change to test results.

@uenoku
Copy link
Copy Markdown
Member

uenoku commented May 12, 2026

I know the context internally but could you mention why the standard lowering (sv.wire + sv.assign) doesn't work?

The if then and else regions can have independent results from each region. The results from both the regions are concatenated.

I think this representation is very non-standard. Can we make sv.ifdef branches have same result types, and return poison from an illegal branch for probe use case?

HW::LowerXMR pass will generate hierarchical reference to the corresponding targets and remove any results from the sv.ifdef.

So it means we cannot legalize ifdef with results in a normal SV pipeline (basically non-Chisel flow)? I understand LowerXMR requires very specific shape of verilog. However in that case I wonder if it's less invasive to introduce a new operation instead of creating a non-emittable sv.ifdef variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants