Skip to content

Commit def39e7

Browse files
uenokuuenoku
authored
[FIRRTL] Add CheckCombLoops handling for InstanceChoiceOp (#9711)
InstanceChoiceOp can reference multiple module alternatives selected at runtime based on options. CheckCombLoops cannot determine which module will be selected, so it conservatively checks all referenced modules (default and alternatives) for combinational loops. This ensures that combinational cycles are detected regardless of which module alternative is selected at runtime. The implementation refactors the existing handleInstanceOp logic into a reusable processInstancePorts helper method to avoid code duplication. * Update InstanceChoiceLoop test cases Removed CHECK-NOT comments for loop detection tests. * Fix name * Update comments
1 parent a51d89f commit def39e7

File tree

3 files changed

+121
-15
lines changed

3 files changed

+121
-15
lines changed

‎lib/Dialect/FIRRTL/FIRRTLUtils.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,7 @@ static void getDeclName(Value value, SmallString<64> &string, bool nameSafe) {
591591
string += op.getInstanceName();
592592
value = nullptr;
593593
})
594-
.Case<InstanceOp, MemOp>([&](auto op) {
594+
.Case<InstanceOp, InstanceChoiceOp, MemOp>([&](auto op) {
595595
string += op.getName();
596596
string += nameSafe ? "_" : ".";
597597
string += op.getPortName(cast<OpResult>(value).getResultNumber());

‎lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ class DiscoverLoops {
128128
.Case<RefForceOp, RefForceInitialOp>(
129129
[&](auto ref) { handleRefForce(ref.getDest(), ref.getSrc()); })
130130
.Case<InstanceOp>([&](auto inst) { handleInstanceOp(inst); })
131+
.Case<InstanceChoiceOp>(
132+
[&](auto inst) { handleInstanceChoiceOp(inst); })
131133
.Case<SubindexOp>([&](SubindexOp sub) {
132134
recordValueRefersToFieldRef(
133135
sub.getInput(),
@@ -328,14 +330,9 @@ class DiscoverLoops {
328330
drivenBy[iter->second].second.push_back(getOrAddNode(srcVal));
329331
}
330332

331-
// Check the referenced module paths and add input ports as the drivers for
332-
// the corresponding output port. The granularity of the connectivity
333-
// relations is per field.
334-
void handleInstanceOp(InstanceOp inst) {
335-
auto refMod = inst.getReferencedModule<FModuleOp>(instanceGraph);
336-
// TODO: External modules not handled !!
337-
if (!refMod)
338-
return;
333+
// Helper to process instance ports for a given module and instance results.
334+
// This is used by both handleInstanceOp and handleInstanceChoiceOp.
335+
void processInstancePorts(FModuleOp refMod, ValueRange instResults) {
339336
auto modulePaths = modulePortPaths.find(refMod);
340337
if (modulePaths == modulePortPaths.end())
341338
return;
@@ -356,12 +353,11 @@ class DiscoverLoops {
356353
auto modSinkPortField = path.first;
357354
auto sinkArgNum =
358355
cast<BlockArgument>(modSinkPortField.getValue()).getArgNumber();
359-
FieldRef sinkPort(inst.getResult(sinkArgNum),
360-
modSinkPortField.getFieldID());
356+
FieldRef sinkPort(instResults[sinkArgNum], modSinkPortField.getFieldID());
361357
auto sinkNode = getOrAddNode(sinkPort);
362358
bool sinkPortIsForceable = false;
363359
if (auto refResultType =
364-
type_dyn_cast<RefType>(inst.getResult(sinkArgNum).getType()))
360+
type_dyn_cast<RefType>(instResults[sinkArgNum].getType()))
365361
sinkPortIsForceable = refResultType.getForceable();
366362

367363
DenseSet<unsigned> setOfEquivalentRWProbes;
@@ -374,11 +370,10 @@ class DiscoverLoops {
374370
if (modSrcPortField == modSinkPortField)
375371
continue;
376372

377-
FieldRef srcPort(inst.getResult(srcArgNum),
378-
modSrcPortField.getFieldID());
373+
FieldRef srcPort(instResults[srcArgNum], modSrcPortField.getFieldID());
379374
bool srcPortIsForceable = false;
380375
if (auto refResultType =
381-
type_dyn_cast<RefType>(inst.getResult(srcArgNum).getType()))
376+
type_dyn_cast<RefType>(instResults[srcArgNum].getType()))
382377
srcPortIsForceable = refResultType.getForceable();
383378
// RWProbes can potentially refer to the same base value. Such ports
384379
// have a path from each other, a false loop, detect such cases.
@@ -433,6 +428,34 @@ class DiscoverLoops {
433428
}
434429
}
435430

431+
// Check the referenced module paths and add input ports as the drivers for
432+
// the corresponding output port. The granularity of the connectivity
433+
// relations is per field.
434+
void handleInstanceOp(InstanceOp inst) {
435+
auto refMod = inst.getReferencedModule<FModuleOp>(instanceGraph);
436+
// Skip if the instance is not a module (e.g. external module).
437+
if (!refMod)
438+
return;
439+
processInstancePorts(refMod, inst.getResults());
440+
}
441+
442+
// For InstanceChoiceOp, conservatively process all possible target modules.
443+
// Since we cannot determine which module will be selected at runtime, we
444+
// must consider combinational paths through all alternatives.
445+
void handleInstanceChoiceOp(InstanceChoiceOp inst) {
446+
// Process all referenced modules (default + alternatives)
447+
for (auto moduleName : inst.getReferencedModuleNamesAttr()) {
448+
auto moduleNameStr = cast<StringAttr>(moduleName);
449+
auto *node = instanceGraph.lookup(moduleNameStr);
450+
if (!node)
451+
continue;
452+
453+
// Skip if the instance is not a module (e.g. external module).
454+
if (auto refMod = dyn_cast<FModuleOp>(*node->getModule()))
455+
processInstancePorts(refMod, inst.getResults());
456+
}
457+
}
458+
436459
// Record the FieldRef, corresponding to the result of the sub op
437460
// `result = base[index]`
438461
void recordValueRefersToFieldRef(Value base, unsigned fieldID, Value result) {

‎test/Dialect/FIRRTL/check-comb-loops.mlir‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,3 +1131,86 @@ firrtl.circuit "DPI" {
11311131
firrtl.matchingconnect %out_0, %0 : !firrtl.uint<8>
11321132
}
11331133
}
1134+
1135+
// -----
1136+
1137+
// Test that InstanceChoiceOp is handled correctly - no loop case
1138+
// CHECK: firrtl.circuit "InstanceChoiceNoLoop"
1139+
firrtl.circuit "InstanceChoiceNoLoop" {
1140+
firrtl.option @Platform {
1141+
firrtl.option_case @FPGA
1142+
firrtl.option_case @ASIC
1143+
}
1144+
1145+
firrtl.module private @DefaultImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1146+
firrtl.matchingconnect %out, %in : !firrtl.uint<8>
1147+
}
1148+
1149+
firrtl.module private @FPGAImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1150+
firrtl.matchingconnect %out, %in : !firrtl.uint<8>
1151+
}
1152+
1153+
firrtl.module @InstanceChoiceNoLoop(in %x: !firrtl.uint<8>, out %y: !firrtl.uint<8>) {
1154+
%inst_in, %inst_out = firrtl.instance_choice inst @DefaultImpl alternatives @Platform {
1155+
@FPGA -> @FPGAImpl
1156+
} (in in: !firrtl.uint<8>, out out: !firrtl.uint<8>)
1157+
firrtl.matchingconnect %inst_in, %x : !firrtl.uint<8>
1158+
firrtl.matchingconnect %y, %inst_out : !firrtl.uint<8>
1159+
}
1160+
}
1161+
1162+
// -----
1163+
1164+
// Test that InstanceChoiceOp detects loops through default module
1165+
firrtl.circuit "InstanceChoiceLoopDefault" {
1166+
firrtl.option @Platform {
1167+
firrtl.option_case @FPGA
1168+
}
1169+
1170+
firrtl.module private @LoopImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1171+
firrtl.matchingconnect %out, %in : !firrtl.uint<8>
1172+
}
1173+
1174+
firrtl.module private @NoLoopImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1175+
%c0_ui8 = firrtl.constant 0 : !firrtl.uint<8>
1176+
firrtl.matchingconnect %out, %c0_ui8 : !firrtl.uint<8>
1177+
}
1178+
1179+
// expected-error @below {{detected combinational cycle in a FIRRTL module, sample path: InstanceChoiceLoopDefault.{inst.in <- y <- inst.out <- inst.in}}
1180+
firrtl.module @InstanceChoiceLoopDefault(in %x: !firrtl.uint<8>) {
1181+
%y = firrtl.wire : !firrtl.uint<8>
1182+
%inst_in, %inst_out = firrtl.instance_choice inst @LoopImpl alternatives @Platform {
1183+
@FPGA -> @NoLoopImpl
1184+
} (in in: !firrtl.uint<8>, out out: !firrtl.uint<8>)
1185+
firrtl.matchingconnect %inst_in, %y : !firrtl.uint<8>
1186+
firrtl.matchingconnect %y, %inst_out : !firrtl.uint<8>
1187+
}
1188+
}
1189+
1190+
// -----
1191+
1192+
// Test that InstanceChoiceOp detects loops through alternative module
1193+
firrtl.circuit "InstanceChoiceLoopAlt" {
1194+
firrtl.option @Platform {
1195+
firrtl.option_case @FPGA
1196+
}
1197+
1198+
firrtl.module private @NoLoopImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1199+
%c0_ui8 = firrtl.constant 0 : !firrtl.uint<8>
1200+
firrtl.matchingconnect %out, %c0_ui8 : !firrtl.uint<8>
1201+
}
1202+
1203+
firrtl.module private @LoopImpl(in %in: !firrtl.uint<8>, out %out: !firrtl.uint<8>) {
1204+
firrtl.matchingconnect %out, %in : !firrtl.uint<8>
1205+
}
1206+
1207+
// expected-error @below {{detected combinational cycle in a FIRRTL module, sample path: InstanceChoiceLoopAlt.{inst.in <- y <- inst.out <- inst.in}}
1208+
firrtl.module @InstanceChoiceLoopAlt(in %x: !firrtl.uint<8>) {
1209+
%y = firrtl.wire : !firrtl.uint<8>
1210+
%inst_in, %inst_out = firrtl.instance_choice inst @NoLoopImpl alternatives @Platform {
1211+
@FPGA -> @LoopImpl
1212+
} (in in: !firrtl.uint<8>, out out: !firrtl.uint<8>)
1213+
firrtl.matchingconnect %inst_in, %y : !firrtl.uint<8>
1214+
firrtl.matchingconnect %y, %inst_out : !firrtl.uint<8>
1215+
}
1216+
}

0 commit comments

Comments
 (0)