Skip to content

[minor] Add Instance Choice#265

Draft
seldridge wants to merge 1 commit intomainfrom
dev/seldridge/instance-choice
Draft

[minor] Add Instance Choice#265
seldridge wants to merge 1 commit intomainfrom
dev/seldridge/instance-choice

Conversation

@seldridge
Copy link
Member

Add instance choice, a new feature that allows for one of several possible instances to be instantiated at Verilog elaboration time.

Add instance choice, a new feature that allows for one of several possible
instances to be instantiated at Verilog elaboration time.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
// Defines for the instance choices
`ifndef __target_Target_foo_x
`define __target_Target_foo_x Bar
`endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment for link.


module Foo();

`__target_Target_foo_x x();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this macro replacement for module name (which requires a additional very specialized operation to HW)? Can we do that with a plain if-def chains?

`ifdef __targetref_Foo_x_a a
`ERROR__targetref_Foo_x_a__must__not__be__set
`else
`define __targetref_Foo_x_a a
Copy link
Collaborator

@uenoku uenoku Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we cannot do emit __targetref in a header if we want to support nested instance choices.

FIRRTL version 5.1.0
circuit Top :
  option Platform :
    FPGA
  option Optimization : 
    Fast
  

  module DefaultTarget : 
    output probe : Probe<UInt<8>> 

    wire r : UInt<8> 
    connect r, UInt<8>(0) 
    define probe = probe(r) 

  module FastTarget : 
    output probe : Probe<UInt<8>> 
    wire w : UInt<8> 
    connect w, UInt<8>(1) 
    define probe = probe(w) 
   
  module FPGATarget : 
    output probe : Probe<UInt<8>> 

    instchoice opt_inst of DefaultTarget, Optimization:
      Fast => FastTarget
    define probe = opt_inst.probe


  public module Top : 
    output probe: Probe<UInt<8>> 
    instchoice platform_inst of DefaultTarget, Platform:
      FPGA => FPGATarget
    define probe = platform_inst.probe
// in targets_Platform_FPGA.svh
`define __targetref_Top_platform_inst_probe `__target_Platform_Top_platform_inst.`__targetref_FPGATarget_opt_inst_probe

__targetref_FPGATarget_opt_inst_probe is defined in targets_Optimization_Fast.svh, and cyclic dependency could be easily introduced if there is another nested instance choice op that went through Optimization -> Platform option path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! Can we break the file inclusion problem by requiring the ordering of the files to be:

  1. Individual instance choice headers which only set instances
  2. and a unified ref header which sets all refs based on instance choices.

Since we've backtracked on the need to hide all instance choices, and instead can handle that with a dedicated tool, this might work. It does mean that the ref header is going to be complex and likely have to mirror the instantiation hierarchy and the options along the way.

Alternatively, (1) could include partial ref paths which are assembled into the final paths in (2).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Individual instance choice headers which only set instances
and a unified ref header which sets all refs based on instance choices.

That is close to what I've implemented so far (= simply inlining paths in ref_*_*.sv files with if-def guards https://gist.github.com/uenoku/d47bd68320dc2cd424ff42df27ed91a4).

The issue with this approach it's necessary to hide specific options/probes from ref files, but yes, it's possible to do with post-process.

Comment on lines +368 to +372
To align with the ABI, this must produce the following files to specialize the circuit for option `A` or option `B`, respectively:

- `targets_Foo_Target_A.sv`
- `targets_Foo_Target_B.sv`

Copy link
Collaborator

@uenoku uenoku Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to generate a header for default target as well since there would be a macro dependency from ref_*.sv to targets_*.sv generally (somehow integration tests in llvm/circt#9815 passed, but I feel it's very tool dependent, maybe verilator is lazily expanding macro?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, having a default is fine. That would need to have some filename that can't collide with the other options to make it work. Not a problem.

Thank you for thinking about this deeply!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i have three options:

  1. Reserve "Default" option (we could generate Default case in parser)
  2. Replace the concept of "Default Module" with "Default Option Case". Basically remove a default module from firrtl.instance_choice, and consider the first Option defined in OptionOp as default (if we want current behaivor Chisel can simply generate "Default" option case for the existing default module).
  3. Actually there is a hacky implementation that doesn't require header file for default case (basically we can get real verilog name instance for default case with inner symbol).

Comment on lines +323 to +325
``` ebnf
filename = "targets_" , module , "_" , target , "_", option , ".vh" ;
```
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to - delimiters to avoid conflicts with FIRRTL identifiers. This is the same trick we played with layer files.

@uenoku
Copy link
Collaborator

uenoku commented Mar 7, 2026

I tried a bit to support instance choices in ChiselSim and I think it's feasible to support for ChiselSim, but
it was already a bit challenging to properly load these header files first even for ChiselSim. I think forcing specific file orders on on each tool feels not scalable.

So i wonder if it makes sense to create a mechanism to configure through single header file declaratively llvm/circt#9870 and force the dependency by `include statement. Do you think if there is a case that this include file mechanism causes some issues?

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.

3 participants