Skip to content

Commit

Permalink
[FIRRTL] FART: allow modules to be in multiple domains at once
Browse files Browse the repository at this point in the history 
When a child module is underneath two different reset domains, the pass
would emit an error message.  This change makes it so that it is allowed
as long as the reset signal name and the reset signal type between the
two domains are the same.

This also changes the logic to re-use a preexisting reset port: if there
is a port with the correct name for a reset domain, but the type does
match, we used to pick a different port name.  This changes it to be an
error, which matches the old SFC behaviour.
  • Loading branch information
@youngar
youngar committed Feb 2, 2025
1 parent 671dec5 commit 767c40b
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 123 deletions.
216 changes: 115 additions & 101 deletions lib/Dialect/FIRRTL/Transforms/InferResets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,50 +55,67 @@ using namespace firrtl;
// Utilities
//===----------------------------------------------------------------------===//

/// Return the name and parent module of a reset. The reset value must either be
/// a module port or a wire/node operation.
static std::pair<StringAttr, FModuleOp> getResetNameAndModule(Value reset) {
if (auto arg = dyn_cast<BlockArgument>(reset)) {
auto module = cast<FModuleOp>(arg.getParentRegion()->getParentOp());
return {module.getPortNameAttr(arg.getArgNumber()), module};
}
auto *op = reset.getDefiningOp();
return {op->getAttrOfType<StringAttr>("name"),
op->getParentOfType<FModuleOp>()};
}

/// Return the name of a reset. The reset value must either be a module port or
/// a wire/node operation.
static StringAttr getResetName(Value reset) {
return getResetNameAndModule(reset).first;
}

namespace {
/// A reset domain.
struct ResetDomain {
/// Whether this is the root of the reset domain.
bool isTop = false;

/// The reset signal for this domain. A null value indicates that this domain
/// explicitly has no reset.
Value reset;

// Implementation details for this domain.
Value existingValue;
Value rootReset;

/// The name of this reset signal.
StringAttr resetName;
/// The type of this reset signal.
Type resetType;

/// Implementation details for this domain. This will be the module local
/// signal for this domain.
Value localReset;
/// If this module already has a port with the matching name, this holds the
/// index of the port.
std::optional<unsigned> existingPort;
StringAttr newPortName;

ResetDomain(Value reset) : reset(reset) {}
/// Create a reset domain without any reset.
ResetDomain() = default;

/// Create a reset domain associated with the root reset.
ResetDomain(Value rootReset)
: rootReset(rootReset), resetName(getResetName(rootReset)),
resetType(rootReset.getType()) {}

/// Returns true if this is in a reset domain, false if this is not a domain.
explicit operator bool() const { return static_cast<bool>(rootReset); }
};
} // namespace

inline bool operator==(const ResetDomain &a, const ResetDomain &b) {
return (a.isTop == b.isTop && a.reset == b.reset);
return (a.isTop == b.isTop && a.resetName == b.resetName &&
a.resetType == b.resetType);
}
inline bool operator!=(const ResetDomain &a, const ResetDomain &b) {
return !(a == b);
}

/// Return the name and parent module of a reset. The reset value must either be
/// a module port or a wire/node operation.
static std::pair<StringAttr, FModuleOp> getResetNameAndModule(Value reset) {
if (auto arg = dyn_cast<BlockArgument>(reset)) {
auto module = cast<FModuleOp>(arg.getParentRegion()->getParentOp());
return {module.getPortNameAttr(arg.getArgNumber()), module};
} else {
auto op = reset.getDefiningOp();
return {op->getAttrOfType<StringAttr>("name"),
op->getParentOfType<FModuleOp>()};
}
}

/// Return the name of a reset. The reset value must either be a module port or
/// a wire/node operation.
static inline StringAttr getResetName(Value reset) {
return getResetNameAndModule(reset).first;
}

/// Construct a zero value of the given type using the given builder.
static Value createZeroValue(ImplicitLocOpBuilder &builder, FIRRTLBaseType type,
SmallDenseMap<FIRRTLBaseType, Value> &cache) {
Expand Down Expand Up @@ -452,8 +469,8 @@ struct InferResetsPass
Value parentReset, InstanceGraph &instGraph,
unsigned indent = 0);

void determineImpl();
void determineImpl(FModuleOp module, ResetDomain &domain);
LogicalResult determineImpl();
LogicalResult determineImpl(FModuleOp module, ResetDomain &domain);

LogicalResult implementFullReset();
LogicalResult implementFullReset(FModuleOp module, ResetDomain &domain);
Expand Down Expand Up @@ -539,7 +556,8 @@ void InferResetsPass::runOnOperationInner() {
return signalPassFailure();

// Determine how each reset shall be implemented.
determineImpl();
if (failed(determineImpl()))
return signalPassFailure();

// Implement the full resets.
if (failed(implementFullReset()))
Expand Down Expand Up @@ -1496,14 +1514,14 @@ LogicalResult InferResetsPass::buildDomains(CircuitOp circuit) {

// Describe the reset domain the instance is in.
note << " is in";
if (domain.reset) {
auto nameAndModule = getResetNameAndModule(domain.reset);
if (domain.rootReset) {
auto nameAndModule = getResetNameAndModule(domain.rootReset);
note << " reset domain rooted at '" << nameAndModule.first.getValue()
<< "' of module '" << nameAndModule.second.getName() << "'";

// Show where the domain reset is declared (once per reset).
if (printedDomainResets.insert(domain.reset).second) {
diag.attachNote(domain.reset.getLoc())
if (printedDomainResets.insert(domain.rootReset).second) {
diag.attachNote(domain.rootReset.getLoc())
<< "reset domain '" << nameAndModule.first.getValue()
<< "' of module '" << nameAndModule.second.getName()
<< "' declared here:";
Expand All @@ -1529,11 +1547,17 @@ void InferResetsPass::buildDomains(FModuleOp module,
});

// Assemble the domain for this module.
ResetDomain domain(parentReset);
ResetDomain domain;
auto it = annotatedResets.find(module);
if (it != annotatedResets.end()) {
// If there is an actual reset, use it for our domain. Otherwise, our
// module is explicitly marked to have no domain.
if (auto localReset = it->second)
domain = ResetDomain(localReset);
domain.isTop = true;
domain.reset = it->second;
} else if (parentReset) {
// Otherwise, we default to using the reset domain of our parent.
domain = ResetDomain(parentReset);
}

// Associate the domain with this module. If the module already has an
Expand All @@ -1551,26 +1575,29 @@ void InferResetsPass::buildDomains(FModuleOp module,
continue;
auto childPath =
instancePathCache->appendInstance(instPath, record->getInstance());
buildDomains(submodule, childPath, domain.reset, instGraph, indent + 1);
buildDomains(submodule, childPath, domain.rootReset, instGraph, indent + 1);
}
}

/// Determine how the reset for each module shall be implemented.
void InferResetsPass::determineImpl() {
LogicalResult InferResetsPass::determineImpl() {
auto anyFailed = false;
LLVM_DEBUG({
llvm::dbgs() << "\n";
debugHeader("Determine implementation") << "\n\n";
});
for (auto &it : domains) {
auto module = cast<FModuleOp>(it.first);
auto &domain = it.second.back().first;
determineImpl(module, domain);
if (failed(determineImpl(module, domain)))
anyFailed = true;
}
return failure(anyFailed);
}

/// Determine how the reset for a module shall be implemented. This function
/// fills in the `existingValue`, `existingPort`, and `newPortName` fields of
/// the given reset domain.
/// fills in the `localReset` and `existingPort` fields of the given reset
/// domain.
///
/// Generally it does the following:
/// - If the domain has explicitly no reset ("ignore"), leaves everything
Expand All @@ -1579,72 +1606,61 @@ void InferResetsPass::determineImpl() {
/// the existing port/wire/node as reset.
/// - If the module already has a port with the reset's name:
/// - If the port has the same type as the reset domain, reuses that port.
/// - Otherwise appends a `_N` suffix with increasing N to create a
/// yet-unused
/// port name, and marks that as to be created.
/// - Otherwise errors out.
/// - Otherwise indicates that a port with the reset's name should be created.
///
void InferResetsPass::determineImpl(FModuleOp module, ResetDomain &domain) {
if (!domain.reset)
return; // nothing to do if the module needs no reset
LogicalResult InferResetsPass::determineImpl(FModuleOp module,
ResetDomain &domain) {
// Nothing to do if the module needs no reset.
if (!domain)
return success();
LLVM_DEBUG(llvm::dbgs() << "Planning reset for " << module.getName() << "\n");

// If this is the root of a reset domain, we don't need to add any ports
// and can just simply reuse the existing values.
if (domain.isTop) {
LLVM_DEBUG(llvm::dbgs() << "- Rooting at local value "
<< getResetName(domain.reset) << "\n");
domain.existingValue = domain.reset;
if (auto blockArg = dyn_cast<BlockArgument>(domain.reset))
LLVM_DEBUG(llvm::dbgs()
<< "- Rooting at local value " << domain.resetName << "\n");
domain.localReset = domain.rootReset;
if (auto blockArg = dyn_cast<BlockArgument>(domain.rootReset))
domain.existingPort = blockArg.getArgNumber();
return;
return success();
}

// Otherwise, check if a port with this name and type already exists and
// reuse that where possible.
auto neededName = getResetName(domain.reset);
auto neededType = domain.reset.getType();
auto neededName = domain.resetName;
auto neededType = domain.resetType;
LLVM_DEBUG(llvm::dbgs() << "- Looking for existing port " << neededName
<< "\n");
auto portNames = module.getPortNames();
auto ports = llvm::zip(portNames, module.getArguments());
auto portIt = llvm::find_if(
ports, [&](auto port) { return std::get<0>(port) == neededName; });
if (portIt != ports.end() && std::get<1>(*portIt).getType() == neededType) {
LLVM_DEBUG(llvm::dbgs()
<< "- Reusing existing port " << neededName << "\n");
domain.existingValue = std::get<1>(*portIt);
domain.existingPort = std::distance(ports.begin(), portIt);
return;
auto *portIt = llvm::find(portNames, neededName);

// If this port does not yet exist, record that we need to create it.
if (portIt == portNames.end()) {
LLVM_DEBUG(llvm::dbgs() << "- Creating new port " << neededName << "\n");
domain.resetName = neededName;
return success();
}

// If we have found a port but the types don't match, pick a new name for
// the reset port.
//
// CAVEAT: The Scala FIRRTL compiler just throws an error in this case. This
// seems unnecessary though, since the compiler can just insert a new reset
// signal as needed.
if (portIt != ports.end()) {
LLVM_DEBUG(llvm::dbgs()
<< "- Existing " << neededName << " has incompatible type "
<< std::get<1>(*portIt).getType() << "\n");
StringAttr newName;
unsigned suffix = 0;
do {
newName =
StringAttr::get(&getContext(), Twine(neededName.getValue()) +
Twine("_") + Twine(suffix++));
} while (llvm::is_contained(portNames, newName));
LLVM_DEBUG(llvm::dbgs()
<< "- Creating uniquified port " << newName << "\n");
domain.newPortName = newName;
return;
LLVM_DEBUG(llvm::dbgs() << "- Reusing existing port " << neededName << "\n");

// If this port has the wrong type, then error out.
auto portNo = std::distance(portNames.begin(), portIt);
auto portType = module.getPortType(portNo);
if (portType != neededType) {
auto diag = emitError(module.getPortLocation(portNo), "module '")
<< module.getName() << "' is in reset domain requiring port '"
<< domain.resetName.getValue() << "' to have type "
<< domain.resetType << ", but has type " << portType;
diag.attachNote(domain.rootReset.getLoc()) << "reset domain rooted here";
return failure();
}

// At this point we know that there is no such port, and we can safely
// create one as needed.
LLVM_DEBUG(llvm::dbgs() << "- Creating new port " << neededName << "\n");
domain.newPortName = neededName;
// We have a pre-existing port which we should use.
domain.existingPort = portNo;
domain.localReset = module.getArgument(portNo);
return success();
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1675,7 +1691,7 @@ LogicalResult InferResetsPass::implementFullReset(FModuleOp module,
<< "\n");

// Nothing to do if the module was marked explicitly with no reset domain.
if (!domain.reset) {
if (!domain) {
LLVM_DEBUG(llvm::dbgs()
<< "- Skipping because module explicitly has no domain\n");
return success();
Expand All @@ -1690,19 +1706,18 @@ LogicalResult InferResetsPass::implementFullReset(FModuleOp module,
annotations.applyToOperation(module);

// If needed, add a reset port to the module.
Value actualReset = domain.existingValue;
if (domain.newPortName) {
PortInfo portInfo{domain.newPortName,
domain.reset.getType(),
auto actualReset = domain.localReset;
if (!domain.localReset) {
PortInfo portInfo{domain.resetName,
domain.resetType,
Direction::In,
{},
domain.reset.getLoc()};
domain.rootReset.getLoc()};
module.insertPorts({{0, portInfo}});
actualReset = module.getArgument(0);
LLVM_DEBUG(llvm::dbgs()
<< "- Inserted port " << domain.newPortName << "\n");
LLVM_DEBUG(llvm::dbgs() << "- Inserted port " << domain.resetName << "\n");
}
assert(actualReset);

LLVM_DEBUG({
llvm::dbgs() << "- Using ";
if (auto blockArg = dyn_cast<BlockArgument>(actualReset))
Expand Down Expand Up @@ -1759,7 +1774,7 @@ LogicalResult InferResetsPass::implementFullReset(FModuleOp module,
emitConnect(builder, wireOp.getResult(), nodeOp.getResult());
resetOp = wireOp;
actualReset = wireOp.getResult();
domain.existingValue = wireOp.getResult();
domain.localReset = wireOp.getResult();
}

// Determine the block into which the reset declaration needs to be
Expand Down Expand Up @@ -1821,20 +1836,19 @@ void InferResetsPass::implementFullReset(Operation *op, FModuleOp module,
if (domainIt == domains.end())
return;
auto &domain = domainIt->second.back().first;
if (!domain.reset)
if (!domain)
return;
LLVM_DEBUG(llvm::dbgs()
<< "- Update instance '" << instOp.getName() << "'\n");

// If needed, add a reset port to the instance.
Value instReset;
if (domain.newPortName) {
if (!domain.localReset) {
LLVM_DEBUG(llvm::dbgs() << " - Adding new result as reset\n");

auto newInstOp = instOp.cloneAndInsertPorts(
{{/*portIndex=*/0,
{domain.newPortName,
type_cast<FIRRTLBaseType>(actualReset.getType()),
{domain.resetName, type_cast<FIRRTLBaseType>(actualReset.getType()),
Direction::In}}});
instReset = newInstOp.getResult(0);

Expand Down
15 changes: 15 additions & 0 deletions test/Dialect/FIRRTL/infer-resets-errors.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,21 @@ firrtl.circuit "top" {
}
}

// -----
// A module in a domain which already has the reset port should error if the
// type of the port is wrong.
firrtl.circuit "Top" {
// expected-error @below {{module 'Child' is in reset domain requiring port 'reset' to have type '!firrtl.asyncreset', but has type '!firrtl.uint<1>'}}
firrtl.module @Child(in %clock: !firrtl.clock, in %reset : !firrtl.uint<1>) {
%reg = firrtl.reg %clock : !firrtl.clock, !firrtl.uint<8>
}
// expected-note @below {{reset domain rooted here}}
firrtl.module @Top(in %clock: !firrtl.clock, in %reset: !firrtl.asyncreset [{class = "circt.FullResetAnnotation", resetType = "async"}]) {
%child_clock, %child_reset = firrtl.instance child @Child(in clock: !firrtl.clock, in reset: !firrtl.uint<1>)
firrtl.connect %child_clock, %clock : !firrtl.clock, !firrtl.clock
}
}

// -----
// Multiple instances of same module cannot live in different reset domains
firrtl.circuit "Top" {
Expand Down
Loading

0 comments on commit 767c40b

Please sign in to comment.