[FIRRTL] Add CheckInstanceChoice pass to reject nested instance choices#9743
[FIRRTL] Add CheckInstanceChoice pass to reject nested instance choices#9743
Conversation
ed9f921 to
740926c
Compare
seldridge
left a comment
There was a problem hiding this comment.
The fact that this is sort of an analysis, but sort of a check feels a bit odd. Did you consider an alternative implementation which directly extended the InstanceInfo analysis with anyInstanceUnderInstanceChoice and allInstancesUnderInstanceChoice and then used that information, coupled with information about being under a layer, to then do the checks and then do the more expensive computations when the checks unexpectedly fail?
With such a formulation, the InstanceInfo likely needs to know about bind vs. inline layers which smells a bit odd, but may be okay. Or: I've been intending to try to have InstanceInfo be the singular place for all information that can be computed via an O(n) walk of the instance graph.
You mention that there are uses for this in LowerLayers where the member functions of the analysis will come into play more.
Good point, I think it's feasible to extend the IntanceInfo (and I realized it's necessary to update it anyway) to check if a module is under choice or not, but I didn't since we need more precise information than boolean.
FIRRTL version 10.0.0
circuit Top:
option Optimization:
Low
High
layer Verification, bind:
module A:
input clock: Clock
node result = UInt<8>(42)
layerblock Verification:
node debug_value = UInt<8>(255)
printf(clock, UInt<1>(1), "Module A: result = %d, debug_value = %d\n", result, debug_value)
module B:
input clock: Clock
public module Top:
input clock: Clock
instchoice b of A, Optimization:
Low => A
High => B
connect b.clock, clock
public module AnotherTop:
input clock: Clock
inst a of A
connect a.clock, clockFor this example module A is used on when I think i can add the utility to InstanceInfo if it's acceptable to run analysis on all public modules. Another direction might be to give colors to modules that restrict usage of options (e.g. module A must be instantiated under option |
7ac590c to
c78b62c
Compare
|
I updated to use InstanceInfo (stacked on #9811). |
Implement tracking of modules that are instantiated within instance choice operations (i.e., modules that are alternatives in an InstanceChoiceOp). This follows the same pattern as other module attributes like `inDesign` and `inEffectiveDesign`. isUnderLayer is updated as well
…figurations This commit introduces a new analysis and verification pass to ensure that instance choices in FIRRTL circuits follow proper nesting rules. The pass prevents nested instance choices with different options on the same path while allowing the same module to be reached through different options on different paths (disjunction is allowed, conjunction is banned).
c78b62c to
7bce037
Compare
7bce037 to
4b3aab5
Compare
This commit introduces a verification pass to ensure that instance choices don't nest, since nested instance choices are challenging to compose with layer bind convention.
For example:
Module A is instantiated by nested instance choice in 5 out of 9 possible settings (
(Platform::default & Optimization::default),(default & High),(ASIC & default),(ASIC & High),(FPGA & Low)). It also has a Verification layer.The bind statement should be elaborated only when these specific settings are met. We could use a macro to describe this logic, but that is too complicated for this first version. Hence this PR bans nested instances so that we can make the condition much simpler (there will be no conjunction
&).InstanceChoiceInfo is introduced to check paths from the top down. If it finds "nested" choices, it will fail.
Right now, InstanceChoiceInfo is not built as a standard MLIR Analysis because it could fail. It will be used later in the LowerLayer to handle these choices.
AI-assisted-by: Augment (Claude Sonnet 4.5)