[InstanceChoice] Add the instance choice specialization pass to firtool #7933

prithayan · 2024-12-02T19:57:41Z

This PR adds the instance choice specialization pass to the firtool pipeline.
Add optional arguments to firtool to select the specialization option for the instance choice.
Then update the pass to use the default module, when no options are specified.
Update lit tests to check for valid instance choice lowering.

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

include/circt/Dialect/FIRRTL/Passes.h

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

uenoku

LGTM, please update capi as well.

seldridge

The roughly sister pass of this is SpecializeLayers and that went a slightly different direction than adding an option. Instead that puts the information directly onto the CircuitOp via a mandatory attribute.

While I can't recall the exact reasons for this design point, consistency between the two approaches may be good.

@youngar or @rwy7: Do you have any thoughts about the above?

seldridge · 2024-12-02T20:29:01Z

include/circt/Firtool/Firtool.h

Please have the PR reflect any changes to this file in the corresponding area of the C-API.

I'm trying to figure out the corresponding type for ArrayRef in C-API, any idea ?

I don't see any uses of ArrayRef for the C-API. This may need to get funneled through an ArrayAttr.

seldridge · 2024-12-02T20:36:45Z

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

            if (it == selected.end()) {
-              inst.emitError("missing specialization for option ")
+              target = inst.getDefaultTargetAttr();
+              inst->emitWarning("missing specialization for option ")
                  << inst.getOptionNameAttr();
-              failed = true;
-              return;
-            }


I realize that this all-or-nothing behavior is originally how this worked.

However, it seems odd to force a user to specialize everything or nothing as opposed to just specializing what the user requested.

In the current implementation, the user need not specialize everything, it would be lowered to the default target, if nothing is specified for an instance choice. The user can specify only the choices that are relevant, and the rest can be lowered to the default target.

seldridge · 2024-12-02T20:43:05Z

test/firtool/instchoice.fir

@@ -26,6 +28,8 @@ circuit Foo:
    ; CHECK-SAME:   @FPGA -> @FPGATarget,
    ; CHECK-SAME:   @ASIC -> @ASICTarget
    ; CHECK-SAME: } (in clock: !firrtl.clock)
+    ; ASIC: hw.instance "inst" @ASICTarget(clock: %clock: !seq.clock) -> ()
+    ; DEFAULT: hw.instance "inst" @DefaultTarget(clock: %clock: !seq.clock) -> ()


🤔 Making the default behavior be "replace instance choices with the default" also seems slightly odd. It seems like the default should be "do nothing". I realize that instance choice is unusable right now unless specialization of every option is run, though.

Maybe it depends on how we define it, the default target, for an instance choice can be defined as the target when no option is specified. Also, if nothing is done here, when should the instance choice be handled in the pipeline ?

If no default is specified, then the instance choice gets lowered to how the FIRRTL ABI defines this lowering. (This is not specified right now. However, I have one such ABI PR'd here: chipsalliance/firrtl-spec#155.)

The underlying problem is that instance choice was only handled for this narrow, internal use case of CIRCT choosing a value for all instance choices as opposed to the more general user request for Verilog elaboration-time selection. 🥲

The motivation here is more about aligning this with how layers are enabled and disabled, e.g., --enable-layer=Verification specializes away the Verification layer, but it leaves any other layers in place. It would seem better to have instance choice specialization work the same way, e.g., --specialize-instance-choice=Performance=ASIC would not affect any other instance choice in the circuit.

We do want to move in this direction. However, we don't have to move there right now with this PR.

WDYT?

I think this makes more sense, but i'm assuming you meant to write:

--specialize-instance-choice=Platform=ASIC

To specify which target is being specialized?

Also, not to derail this (and i see there was some discussion of this in the spec PR) but it is useful to be able to specialize different instances of the same instance_choice to write miter-style circuits for testing the choices themselves (esp. when they are used in part of a larger circuit under test). I guess here it might just be fine to do the specialization at VCS-compile time though. For this reason i also like that default be explicitly named (as also discussed in the spec PR).

So in the short run, we'd need a flag to achieve the effect of specializing all modules to their default and just provide that when we're passing in circuits that contain instance choices for now?

I think this makes more sense, but i'm assuming you meant to write:

--specialize-instance-choice=Platform=ASIC

To specify which target is being specialized?

Yes!

it is useful to be able to specialize different instances of the same instance_choice to write miter-style circuits for testing the choices themselves (esp. when they are used in part of a larger circuit under test)

Per-instance specialization currently works. It does create a blow-up in the number of options. This is an intentional choice that complicates the design. E.g., consider instance choice used in a multicore design to replace a core with a model. The designer needs to decide what swapping flexibility they want to have. If they want to have complete flexibility to swap every core arbitrarily in an n-core design, that would have to be n-different options. They likely don't want that and will choose to have one instance choice which swaps all of the cores except one.

I'd prefer to not support arbitrary per-instance after-the-fact specialization as that amounts to mandated module duplication which then has interactions with must-dedup and designer expectations around deduplication. I'm more comfortable with this if CIRCT-mandated deduplication (must-dedup) is removed.

So in the short run, we'd need a flag to achieve the effect of specializing all modules to their default and just provide that when we're passing in circuits that contain instance choices for now?

I'm good with either:

All instance choice specializations are specified up front.

There is a catch-all -specialize-to-default-if-no-override which will, after applying selected choices, will then convert everything to the default choice.

After more thought, i think strictness of (1) is probably desirable to avoid accidental footguns? We can always add the flag later. I'll defer to you and @prithayan on this one.

I've decided to go with the option 1, unless all the instance choice specializations are specified, the pass will error out.
Once the lowering of instance choice through the pipeline to verilog is supported, we can get rid of the error.

So if you went with (1) how do you specialize to Default?

prithayan · 2024-12-04T16:30:27Z

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

@@ -89,12 +85,24 @@ struct SpecializeOptionPass
          });
        });

+    bool analysisPreserved = numInstances == 0;
+    circuit->walk([&](OptionOp optionOp) {
+      optionOp->erase();


@seldridge , @uenoku , just wanted to make sure, this makes sense right ?
The OptionOp wouldn't be useful for any other passes downstream, even if we ignore an InstanceChoiceOp, when it is not specialized, and left in IR after this pass.

This would only be safe for options that were specialized. If any InstanceChoiceOp is left in, e.g., if the circuit is partially instance-choice specialized, then this would create an invalid symbol reference.

Yes, that makes sense. But since, there are no other passes that can handle the OptionOp maybe its okay to delete them. And this pass also errors out on partially specified instance choice. WDYT ?

All the other passes are supposed to handle these things, just it's not implemented, yet. 🫠 The concern is more about if this pass is guaranteed to specialize away everything (which we eventually don't want), then it's fine. However, if it is going to leave anything in, then it shouldn't delete everything.

Right, for now, this pass currently doesn't leave any InstanceChoiceOp, it should error out if a choice is not selected. So, it should be fine to delete it.
But the first pass that needs to handle OptionOp is LowerToHW, there is no HW counterpart for it.

Yep... just showing something end-to-end would be cool. FWIW, I am totally fine with LowerToHW erroring with "unsupported operation". That's all part of getting a feature turned on incrementally. 😉

prithayan · 2024-12-04T23:31:15Z

PR is ready for another review, @uenoku , @seldridge please take a look if you get a chance.

I updated the select option to follow the specialize layers style.
Removed the lowering to default and instead error out on partially specified choice.

seldridge

Nice. Some comments/nits throughout.

Can the PR add a test of the error behavior if the specialization is not fully specified?

lib/Dialect/FIRRTL/Import/FIRParser.cpp

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

lib/Firtool/Firtool.cpp

test/Dialect/FIRRTL/specialize-option-errors.mlir

prithayan · 2024-12-05T15:53:18Z

Thanks for the review and discussion everyone. I'm planning to split this into a separate PR for the parser changes and will merge them as two commits.

…ol pipeline

seldridge · 2024-12-05T18:50:57Z

Thanks for the review and discussion everyone. I'm planning to split this into a separate PR for the parser changes and will merge them as two commits.

👍 Always feel empowered to slice things up into smaller commit chunks. 💯

prithayan requested review from darthscsi and seldridge as code owners December 2, 2024 19:57

prithayan requested review from uenoku, seldridge and darthscsi and removed request for seldridge and darthscsi December 2, 2024 19:57

uenoku reviewed Dec 2, 2024

View reviewed changes

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp Show resolved Hide resolved

include/circt/Dialect/FIRRTL/Passes.h Outdated Show resolved Hide resolved

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp Outdated Show resolved Hide resolved

uenoku approved these changes Dec 2, 2024

View reviewed changes

seldridge reviewed Dec 2, 2024

View reviewed changes

prithayan force-pushed the dev/pbarua/instance-choice branch from 97e1d40 to 10b0ec9 Compare December 4, 2024 15:30

prithayan commented Dec 4, 2024

View reviewed changes

prithayan force-pushed the dev/pbarua/instance-choice branch 2 times, most recently from f57693f to 393da2d Compare December 4, 2024 23:22

seldridge approved these changes Dec 5, 2024

View reviewed changes

prithayan force-pushed the dev/pbarua/instance-choice branch from 393da2d to 5034c95 Compare December 5, 2024 15:18

prithayan mentioned this pull request Dec 5, 2024

[FirParser] Add instance choice selection as circt attribute #7951

Merged

prithayan added 9 commits December 5, 2024 10:18

[InstanceChoice] Add the instance choice specialization pass to firto…

c86dadd

…ol pipeline

Remove the default lowering of instance choice

86b448b

Move the select option to parser and circt attribute

942aff5

Move the select option to circt attribute

a2f50c2

Move the specialize options pass next to specialize layers

ef489af

clang-format

a1729c3

cleanup

a0456b2

Address reiview comments

43463f1

Update lit tests with partially specified option

383b91b

prithayan force-pushed the dev/pbarua/instance-choice branch from 5034c95 to 383b91b Compare December 5, 2024 18:38

prithayan merged commit f882164 into main Dec 5, 2024
4 checks passed

prithayan deleted the dev/pbarua/instance-choice branch December 5, 2024 20:55

prithayan mentioned this pull request Dec 5, 2024

[InstanceChoice] Add a default override for unspecified options #7955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstanceChoice] Add the instance choice specialization pass to firtool #7933

[InstanceChoice] Add the instance choice specialization pass to firtool #7933

prithayan commented Dec 2, 2024

uenoku left a comment

seldridge left a comment

seldridge Dec 2, 2024

prithayan Dec 4, 2024

seldridge Dec 4, 2024

seldridge Dec 2, 2024

prithayan Dec 2, 2024

seldridge Dec 2, 2024

prithayan Dec 2, 2024

seldridge Dec 2, 2024

davidbiancolin Dec 2, 2024

davidbiancolin Dec 2, 2024

seldridge Dec 2, 2024

davidbiancolin Dec 3, 2024

prithayan Dec 4, 2024

davidbiancolin Dec 5, 2024

prithayan Dec 4, 2024

seldridge Dec 4, 2024

prithayan Dec 4, 2024

seldridge Dec 4, 2024

prithayan Dec 5, 2024

seldridge Dec 5, 2024

prithayan commented Dec 4, 2024

seldridge left a comment

prithayan commented Dec 5, 2024

seldridge commented Dec 5, 2024

[InstanceChoice] Add the instance choice specialization pass to firtool #7933

[InstanceChoice] Add the instance choice specialization pass to firtool #7933

Conversation

prithayan commented Dec 2, 2024

uenoku left a comment

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prithayan commented Dec 4, 2024

seldridge left a comment

Choose a reason for hiding this comment

prithayan commented Dec 5, 2024

seldridge commented Dec 5, 2024