Skip to content

[FIRRTL] Remove convention verification for InstanceChoice #10066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
uenoku merged 4 commits into from
Mar 28, 2026
Merged

[FIRRTL] Remove convention verification for InstanceChoice #10066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aae098f
[FIRRTL] Remove convention verification
uenoku Mar 27, 2026
6e59066
[FIRRTL] Rename instance choice scalarization helper
uenoku Mar 28, 2026
2fb3250
Update comments
uenoku Mar 28, 2026
d2dab7a
Update comments
uenoku Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/circt/Dialect/FIRRTL/FIRRTLUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#ifndef CIRCT_DIALECT_FIRRTL_FIRRTLUTILS_H
#define CIRCT_DIALECT_FIRRTL_FIRRTLUTILS_H

#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
#include "circt/Dialect/FIRRTL/FIRRTLOps.h"
#include "mlir/IR/BuiltinOps.h"

Expand Down
9 changes: 0 additions & 9 deletions lib/Dialect/FIRRTL/FIRRTLOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3165,7 +3165,6 @@ LogicalResult InstanceChoiceOp::verify() {
LogicalResult
InstanceChoiceOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
auto caseNames = getCaseNamesAttr();
std::optional<Convention> convention;
for (auto moduleName : getModuleNamesAttr()) {
auto moduleNameRef = cast<FlatSymbolRefAttr>(moduleName);
if (failed(instance_like_impl::verifyReferencedModule(*this, symbolTable,
Expand All @@ -3178,14 +3177,6 @@ InstanceChoiceOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
if (isa<FIntModuleOp>(referencedModule))
return emitOpError("intmodule must be instantiated with instance op, "
"not via 'firrtl.instance_choice'");

if (!convention) {
convention = referencedModule.getConvention();
continue;
}

if (*convention != referencedModule.getConvention())
return emitOpError("all modules must have the same convention");
}

auto root = cast<SymbolRefAttr>(caseNames[0]).getRootReference();
Expand Down
1 change: 1 addition & 0 deletions lib/Dialect/FIRRTL/FIRRTLUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//

#include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
#include "circt/Dialect/HW/HWOps.h"
#include "circt/Dialect/HW/InnerSymbolNamespace.h"
#include "circt/Dialect/Seq/SeqTypes.h"
Expand Down
13 changes: 11 additions & 2 deletions lib/Dialect/FIRRTL/Transforms/LowerSignatures.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "circt/Support/InstanceGraphInterface.h"
#include "mlir/IR/Threading.h"
#include "mlir/Pass/Pass.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Debug.h"

#define DEBUG_TYPE "firrtl-lower-signatures"
Expand Down Expand Up @@ -508,6 +509,7 @@ struct LowerSignaturesPass
// This is the main entrypoint for the lowering pass.
void LowerSignaturesPass::runOnOperation() {
CIRCT_DEBUG_SCOPED_PASS_LOGGER(this);
auto &instanceGraph = getAnalysis<InstanceGraph>();

// Cached attr
AttrCache cache(&getContext());
Expand All @@ -516,8 +518,15 @@ void LowerSignaturesPass::runOnOperation() {
auto circuit = getOperation();

for (auto mod : circuit.getOps<FModuleLike>()) {
if (lowerModuleSignature(mod, mod.getConvention(), cache,
portMap[mod.getNameAttr()])
auto convention = mod.getConvention();
// Instance choices select between modules with a shared port shape, so
// any module instantiated by one must use the scalarized convention.
if (llvm::any_of(instanceGraph.lookup(mod)->uses(),
[](InstanceRecord *use) {
return use->getInstance<InstanceChoiceOp>();
}))
convention = Convention::Scalarized;
if (lowerModuleSignature(mod, convention, cache, portMap[mod.getNameAttr()])
.failed())
return signalPassFailure();
}
Expand Down
12 changes: 11 additions & 1 deletion lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "mlir/Pass/Pass.h"
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/BitVector.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Debug.h"

#define DEBUG_TYPE "firrtl-lower-types"
Expand Down Expand Up @@ -1828,6 +1829,7 @@ void LowerTypesPass::runOnOperation() {
CIRCT_DEBUG_SCOPED_PASS_LOGGER(this);

std::vector<FModuleLike> ops;
auto &instanceGraph = getAnalysis<InstanceGraph>();
// Symbol Table
auto &symTbl = getAnalysis<SymbolTable>();
Copy link
Copy Markdown
Member Author

@uenoku uenoku Mar 27, 2026

Choose a reason for hiding this comment

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

considered to replace symTbl with instanceGraph but it was challenging due to parallelization and instance op mutation.

// Cached attr
Expand All @@ -1836,7 +1838,15 @@ void LowerTypesPass::runOnOperation() {
DenseMap<FModuleLike, Convention> conventionTable;
auto circuit = getOperation();
for (auto module : circuit.getOps<FModuleLike>()) {
conventionTable.insert({module, module.getConvention()});
auto convention = module.getConvention();
// Instance choices select between modules with a shared port shape, so
// any module instantiated by one must use the scalarized convention.
if (llvm::any_of(instanceGraph.lookup(module)->uses(),
[](InstanceRecord *use) {
return use->getInstance<InstanceChoiceOp>();
}))
convention = Convention::Scalarized;
conventionTable.insert({module, convention});
ops.push_back(module);
}

Expand Down
24 changes: 0 additions & 24 deletions test/Dialect/FIRRTL/errors.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3485,27 +3485,3 @@ firrtl.circuit "DomainCreateWrongFieldType" {
// expected-error @-1 {{use of value '%period' expects different type than prior uses: '!firrtl.integer' vs '!firrtl.string'}}
}
}

// -----

firrtl.circuit "InstanceChoiceAggregate" {
firrtl.option @Platform {
firrtl.option_case @FPGA
}
firrtl.module private @Target(
in %in: !firrtl.vector<uint<8>, 2>,
out %out: !firrtl.vector<uint<8>, 2>
) attributes {convention = #firrtl<convention internal>} { }

firrtl.module public @PublicTarget(
in %in: !firrtl.vector<uint<8>, 2>,
out %out: !firrtl.vector<uint<8>, 2>
) attributes {convention = #firrtl<convention scalarized>}{ }

firrtl.module @InstanceChoiceAggregate() {
// expected-error @below {{'firrtl.instance_choice' op all modules must have the same convention}}
%inst_in, %inst_out = firrtl.instance_choice inst sym @sym @Target alternatives @Platform {
@FPGA -> @PublicTarget
} (in in: !firrtl.vector<uint<8>, 2>, out out: !firrtl.vector<uint<8>, 2>)
}
}
11 changes: 5 additions & 6 deletions test/Dialect/FIRRTL/lower-signatures.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,13 @@ firrtl.circuit "InstanceChoice" {
firrtl.module @TargetWithDomain(
in %D: !firrtl.domain<@ClockDomain()>,
in %b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [%D]
) attributes {convention = #firrtl<convention scalarized>} {
) {
}

firrtl.module @FPGATargetWithDomain(
in %D: !firrtl.domain<@ClockDomain()>,
in %b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [%D]
) attributes {convention = #firrtl<convention scalarized>} {
}
firrtl.extmodule @FPGATargetWithDomain(
in D: !firrtl.domain<@ClockDomain()>,
in b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [D]
)

firrtl.module @ASICTargetWithDomain(
in %D: !firrtl.domain<@ClockDomain()>,
Expand Down
8 changes: 6 additions & 2 deletions test/Dialect/FIRRTL/lower-types.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1568,6 +1568,10 @@ firrtl.circuit "InstanceChoiceTest" {
firrtl.matchingconnect %1, %0 : !firrtl.uint<8>
}

firrtl.extmodule private @Ext(
in in: !firrtl.bundle<a: uint<8>, b: uint<8>>,
out out: !firrtl.bundle<x: uint<8>, y: uint<8>>
)

// CHECK-LABEL: firrtl.module @InstanceChoiceTest
firrtl.module @InstanceChoiceTest(
Expand All @@ -1576,10 +1580,10 @@ firrtl.circuit "InstanceChoiceTest" {
out %out_x: !firrtl.uint<8>,
out %out_y: !firrtl.uint<8>
) {
// CHECK: %inst_in_a, %inst_in_b, %inst_out_x, %inst_out_y = firrtl.instance_choice inst @TargetModule alternatives @Platform
// CHECK: %inst_in_a, %inst_in_b, %inst_out_x, %inst_out_y = firrtl.instance_choice inst @Ext alternatives @Platform
// CHECK-SAME: @FPGA -> @TargetModule
// CHECK-SAME: (in in_a: !firrtl.uint<8>, in in_b: !firrtl.uint<8>, out out_x: !firrtl.uint<8>, out out_y: !firrtl.uint<8>)
%inst_in, %inst_out = firrtl.instance_choice inst @TargetModule alternatives @Platform {
%inst_in, %inst_out = firrtl.instance_choice inst @Ext alternatives @Platform {
@FPGA -> @TargetModule
} (in in: !firrtl.bundle<a: uint<8>, b: uint<8>>, out out: !firrtl.bundle<x: uint<8>, y: uint<8>>)

Expand Down
Loading