Skip to content
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

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

Merged
merged 2 commits into from
Dec 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ec6b74
[InstanceChoice] Add a default override for unspecified options
prithayan Dec 5, 2024
6f8dd2d
Rename the CLI and variable
prithayan Dec 5, 2024
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
3 changes: 3 additions & 0 deletions include/circt-c/Firtool/Firtool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ MLIR_CAPI_EXPORTED void
circtFirtoolOptionsSetDisableCSEinClasses(CirctFirtoolFirtoolOptions options,
bool value);

MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetSelectDefaultInstanceChoice(
CirctFirtoolFirtoolOptions options, bool value);

//===----------------------------------------------------------------------===//
// Populate API.
//===----------------------------------------------------------------------===//
Expand Down
3 changes: 2 additions & 1 deletion include/circt/Dialect/FIRRTL/Passes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ std::unique_ptr<mlir::Pass> createProbesToSignalsPass();

std::unique_ptr<mlir::Pass> createSpecializeLayersPass();

std::unique_ptr<mlir::Pass> createSpecializeOptionPass();
std::unique_ptr<mlir::Pass>
createSpecializeOptionPass(bool selectDefault = false);

std::unique_ptr<mlir::Pass> createCreateCompanionAssume();

Expand Down
4 changes: 4 additions & 0 deletions include/circt/Dialect/FIRRTL/Passes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,10 @@ def SpecializeOption : Pass<"firrtl-specialize-option", "firrtl::CircuitOp"> {
}];
let constructor = "circt::firrtl::createSpecializeOptionPass()";

let options = [
Option<"selectDefault", "specialize-to-default-if-no-override", "bool", "false",
Copy link
Member

Choose a reason for hiding this comment

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

Can the option name (both CLI and variable) be more descriptive than just "specialize"? This is a term that is used also for layers and could mean (roughly) and removal of a parameter. Or: include instance choice in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed both to be a bit more descriptive. Hopefully its not too long,
select-default-for-unspecified-instance-choice

Copy link
Member

Choose a reason for hiding this comment

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

SGTM. Overly descriptive and obvious is fine-by-me. 💯

"Specialize instance choice to default, if no option selected.">,
];
let statistics = [
Statistic<"numInstances", "num-instances",
"Number of instances specialized">
Expand Down
9 changes: 9 additions & 0 deletions include/circt/Firtool/Firtool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ class FirtoolOptions {
bool shouldFixupEICGWrapper() const { return fixupEICGWrapper; }
bool shouldAddCompanionAssume() const { return addCompanionAssume; }
bool shouldDisableCSEinClasses() const { return disableCSEinClasses; }
bool shouldSelectDefaultInstanceChoice() const {
return selectDefaultInstanceChoice;
}

// Setters, used by the CAPI
FirtoolOptions &setOutputFilename(StringRef name) {
Expand Down Expand Up @@ -362,6 +365,11 @@ class FirtoolOptions {
return *this;
}

FirtoolOptions &setSelectDefaultInstanceChoice(bool value) {
selectDefaultInstanceChoice = value;
return *this;
}

private:
std::string outputFilename;
bool disableAnnotationsUnknown;
Expand Down Expand Up @@ -409,6 +417,7 @@ class FirtoolOptions {
bool fixupEICGWrapper;
bool addCompanionAssume;
bool disableCSEinClasses;
bool selectDefaultInstanceChoice;
};

void registerFirtoolCLOptions();
Expand Down
5 changes: 5 additions & 0 deletions lib/CAPI/Firtool/Firtool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ void circtFirtoolOptionsSetDisableCSEinClasses(
unwrap(options)->setDisableCSEinClasses(value);
}

void circtFirtoolOptionsSetSelectDefaultInstanceChoice(
CirctFirtoolFirtoolOptions options, bool value) {
unwrap(options)->setSelectDefaultInstanceChoice(value);
}

//===----------------------------------------------------------------------===//
// Populate API.
//===----------------------------------------------------------------------===//
Expand Down
32 changes: 20 additions & 12 deletions lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace {
struct SpecializeOptionPass
: public circt::firrtl::impl::SpecializeOptionBase<SpecializeOptionPass> {
using SpecializeOptionBase::numInstances;
using SpecializeOptionBase::selectDefault;

void runOnOperation() override {
auto circuit = getOperation();
Expand Down Expand Up @@ -66,20 +67,25 @@ struct SpecializeOptionPass
&getContext(), circuit.getOps<FModuleOp>(), [&](auto module) {
module.walk([&](firrtl::InstanceChoiceOp inst) {
auto it = selected.find(inst.getOptionNameAttr());
FlatSymbolRefAttr target;
if (it == selected.end()) {
inst.emitError("missing specialization for option ")
<< inst.getOptionNameAttr();
failed = true;
return;
}
if (!selectDefault) {
inst.emitError("missing specialization for option ")
<< inst.getOptionNameAttr();
failed = true;
return;
}
target = inst.getDefaultTargetAttr();
} else
target = inst.getTargetOrDefaultAttr(it->second);

ImplicitLocOpBuilder builder(inst.getLoc(), inst);
auto newInst = builder.create<InstanceOp>(
inst->getResultTypes(), inst.getTargetOrDefaultAttr(it->second),
inst.getNameAttr(), inst.getNameKindAttr(),
inst.getPortDirectionsAttr(), inst.getPortNamesAttr(),
inst.getAnnotationsAttr(), inst.getPortAnnotationsAttr(),
builder.getArrayAttr({}), UnitAttr{}, inst.getInnerSymAttr());
inst->getResultTypes(), target, inst.getNameAttr(),
inst.getNameKindAttr(), inst.getPortDirectionsAttr(),
inst.getPortNamesAttr(), inst.getAnnotationsAttr(),
inst.getPortAnnotationsAttr(), builder.getArrayAttr({}),
UnitAttr{}, inst.getInnerSymAttr());
inst.replaceAllUsesWith(newInst);
inst.erase();

Expand All @@ -101,6 +107,8 @@ struct SpecializeOptionPass
};
} // namespace

std::unique_ptr<Pass> firrtl::createSpecializeOptionPass() {
return std::make_unique<SpecializeOptionPass>();
std::unique_ptr<Pass> firrtl::createSpecializeOptionPass(bool selectDefault) {
auto pass = std::make_unique<SpecializeOptionPass>();
pass->selectDefault = selectDefault;
return pass;
}
12 changes: 10 additions & 2 deletions lib/Firtool/Firtool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ LogicalResult firtool::populateCHIRRTLToLowFIRRTL(mlir::PassManager &pm,

// Must run the specialize instance-choice and layers passes after all
// diagnostic passes have run, otherwise it can hide errors.
pm.addNestedPass<firrtl::CircuitOp>(firrtl::createSpecializeOptionPass());
pm.addNestedPass<firrtl::CircuitOp>(firrtl::createSpecializeOptionPass(
opt.shouldSelectDefaultInstanceChoice()));
pm.addNestedPass<firrtl::CircuitOp>(firrtl::createSpecializeLayersPass());

// Run after inference, layer specialization.
Expand Down Expand Up @@ -733,6 +734,12 @@ struct FirtoolCmdOptions {
"add-companion-assume",
llvm::cl::desc("Add companion assumes to assertions"),
llvm::cl::init(false)};

llvm::cl::opt<bool> selectDefaultInstanceChoice{
"specialize-to-default-if-no-override",
llvm::cl::desc(
"Specialize instance choice to default, if no option selected"),
llvm::cl::init(false)};
};
} // namespace

Expand Down Expand Up @@ -769,7 +776,7 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
ckgEnableName("en"), ckgTestEnableName("test_en"), ckgInstName("ckg"),
exportModuleHierarchy(false), stripFirDebugInfo(true),
stripDebugInfo(false), fixupEICGWrapper(false), addCompanionAssume(false),
disableCSEinClasses(false) {
disableCSEinClasses(false), selectDefaultInstanceChoice(false) {
if (!clOptions.isConstructed())
return;
outputFilename = clOptions->outputFilename;
Expand Down Expand Up @@ -818,4 +825,5 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
stripDebugInfo = clOptions->stripDebugInfo;
fixupEICGWrapper = clOptions->fixupEICGWrapper;
addCompanionAssume = clOptions->addCompanionAssume;
selectDefaultInstanceChoice = clOptions->selectDefaultInstanceChoice;
}
4 changes: 3 additions & 1 deletion test/firtool/instchoice.fir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
; RUN: firtool %s --parse-only --select-instance-choice=Platform=ASIC | FileCheck %s
; RUN: firtool %s --ir-hw --disable-opt --select-instance-choice=Platform=ASIC | FileCheck %s --check-prefix=ASIC
; RUN: firtool %s --ir-hw --disable-opt --specialize-to-default-if-no-override | FileCheck %s --check-prefixes=DEFAULT

FIRRTL version 4.1.0
; CHECK: firrtl.circuit "Foo" attributes {select_inst_choice = ["Platform=ASIC"]} {
Expand Down Expand Up @@ -28,7 +29,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) -> ()
; ASIC: hw.instance "inst" @ASICTarget
; DEFAULT: hw.instance "inst" @DefaultTarget
instchoice inst of DefaultTarget, Platform :
FPGA => FPGATarget
ASIC => ASICTarget
Expand Down
Loading