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 1 commit
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
4 changes: 4 additions & 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 Expand Up @@ -90,6 +91,9 @@ class InstanceChoiceMacroTable {
llvm::MapVector<std::pair<StringAttr, StringAttr>, FlatSymbolRefAttr> cache;
};

/// Return true if the module instance graph node requires scalarization.
bool isScalarizeRequired(InstanceGraphNode *node);
Copy link
Copy Markdown
Member

@seldridge seldridge Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is really a "is this instantiated by instance choice" which then means, only elsewhere, this should be scalarized.

At minimum, this would be better if renamed.

Going further, this feels like something that would be better handled in the InstanceInfo analysis which it could then just return. This would also be more efficient for repeated calls (though I don't think we do have repeated calls).

Copy link
Copy Markdown
Member Author

@uenoku uenoku Mar 28, 2026

Choose a reason for hiding this comment

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

Fair point. In that case I think it's better not to expose it to a header so I simply inlined the logic with comment.


//===----------------------------------------------------------------------===//
// Template utilities
//===----------------------------------------------------------------------===//
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
7 changes: 7 additions & 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 Expand Up @@ -1254,3 +1255,9 @@ circt::firrtl::InstanceChoiceMacroTable::getMacro(StringAttr optionName,
return {};
return it->second;
}

bool circt::firrtl::isScalarizeRequired(InstanceGraphNode *node) {
return llvm::any_of(node->uses(), [](InstanceRecord *use) {
return use->getInstance<InstanceChoiceOp>();
});
}
7 changes: 5 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 @@ -508,6 +508,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 +517,10 @@ 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();
if (isScalarizeRequired(instanceGraph.lookup(mod)))
convention = Convention::Scalarized;
if (lowerModuleSignature(mod, convention, cache, portMap[mod.getNameAttr()])
.failed())
return signalPassFailure();
}
Expand Down
6 changes: 5 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 @@ -1828,6 +1828,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 +1837,10 @@ 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();
if (isScalarizeRequired(instanceGraph.lookup(module)))
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