Skip to content

Commit

Permalink
[FIRRTL][LowerToHW] Copy InnerSymAttrs to lowered ops (#5928)
Browse files Browse the repository at this point in the history 
This change ensures that when lowering InnerSymbol operations, we copy the
existing inner symbol to the target, as well as properly adding a symbol on
fieldID 0 when required.  This fixes an assumption of symbol name uniqueness,
and they are now uniqued in the module's symbol namespace. A side effect of
this change is that the used symbol names are always of the form `@sym_1`.
This also fixes a bug where an `InnerSymAttr` was assumed to target field ID 0,
and could result in an incorrect lowering of the `DontTouch` annotation.
  • Loading branch information
@youngar
youngar authored Aug 23, 2023
1 parent bdec2b9 commit 56e98be
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 105 deletions.
14 changes: 14 additions & 0 deletions include/circt/Dialect/FIRRTL/FIRRTLUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,24 @@ Value getValueByFieldID(ImplicitLocOpBuilder builder, Value value,
/// leaf ground type.
void walkGroundTypes(FIRRTLType firrtlType,
llvm::function_ref<void(uint64_t, FIRRTLBaseType)> fn);

//===----------------------------------------------------------------------===//
// Inner symbol and InnerRef helpers.
//===----------------------------------------------------------------------===//

/// Ensure that the the InnerSymAttr has a symbol on the field specified.
/// Returns the updated InnerSymAttr as well as the name of the symbol attached
/// to the specified field.
std::pair<hw::InnerSymAttr, StringAttr>
getOrAddInnerSym(MLIRContext *context, hw::InnerSymAttr attr, uint64_t fieldID,
llvm::function_ref<hw::InnerSymbolNamespace &()> getNamespace);

/// Returns an inner symbol identifier for the specified target (op or port),
/// adding one if necessary.
StringAttr
getOrAddInnerSym(const hw::InnerSymTarget &target,
llvm::function_ref<hw::InnerSymbolNamespace &()> getNamespace);

using GetNamespaceCallback =
llvm::function_ref<hw::InnerSymbolNamespace &(FModuleLike mod)>;

Expand Down
7 changes: 3 additions & 4 deletions include/circt/Dialect/HW/HWMiscOps.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,13 @@ def WireOp : HWOp<"wire", [
let builders = [
OpBuilder<(ins "mlir::Value":$input,
CArg<"const StringAttrOrRef &", "{}">:$name,
CArg<"const StringAttrOrRef &", "{}">:$innerSym), [{
CArg<"hw::InnerSymAttr", "{}">:$innerSym), [{
auto *context = odsBuilder.getContext();
odsState.addOperands(input);
if (auto attr = name.get(context))
odsState.addAttribute(getNameAttrName(odsState.name), attr);
if (auto attr = innerSym.get(context))
odsState.addAttribute(getInnerSymAttrName(odsState.name),
InnerSymAttr::get(attr));
if (innerSym)
odsState.addAttribute(getInnerSymAttrName(odsState.name), innerSym);
odsState.addTypes(input.getType());
}]>
];
Expand Down
92 changes: 40 additions & 52 deletions lib/Conversion/FIRRTLToHW/LowerToHW.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1449,6 +1449,33 @@ struct FIRRTLLowering : public FIRRTLVisitor<FIRRTLLowering, LogicalResult> {
Backedge createBackedge(Value orig, Type type);
bool updateIfBackedge(Value dest, Value src);

/// Returns true if the lowered operation requires an inner symbol on it.
bool requiresInnerSymbol(hw::InnerSymbolOpInterface op) {
if (AnnotationSet::removeDontTouch(op))
return true;
if (!hasDroppableName(op))
return true;
if (auto forceable = dyn_cast<Forceable>(op.getOperation()))
if (forceable.isForceable())
return true;
return false;
}

/// Gets the lowered InnerSymAttr of this operation. If the operation is
/// DontTouched, has a non-droppable name, or is forceable, then we will
/// ensure that the InnerSymAttr has a symbol with fieldID zero.
hw::InnerSymAttr lowerInnerSymbol(hw::InnerSymbolOpInterface op) {
auto attr = op.getInnerSymAttr();
// TODO: we should be checking for symbol collisions here and renaming as
// neccessary. As well, we should record the renamings in a map so that we
// can update any InnerRefAttrs that we find.
if (requiresInnerSymbol(op))
std::tie(attr, std::ignore) = getOrAddInnerSym(
op.getContext(), attr, 0,
[&]() -> hw::InnerSymbolNamespace & { return moduleNamespace; });
return attr;
}

void runWithInsertionPointAtEndOfBlock(std::function<void(void)> fn,
Region &region);

Expand Down Expand Up @@ -1707,7 +1734,7 @@ struct FIRRTLLowering : public FIRRTLVisitor<FIRRTLLowering, LogicalResult> {
llvm::SmallDenseMap<std::pair<Block *, Attribute>, sv::IfDefOp> ifdefBlocks;
llvm::SmallDenseMap<Block *, sv::InitialOp> initialBlocks;

/// A namespace that can be used to generte new symbol names that are unique
/// A namespace that can be used to generate new symbol names that are unique
/// within this module.
hw::InnerSymbolNamespace moduleNamespace;

Expand Down Expand Up @@ -2769,26 +2796,12 @@ LogicalResult FIRRTLLowering::visitDecl(WireOp op) {
return setLowering(op.getResult(), Value());

// Name attr is required on sv.wire but optional on firrtl.wire.
StringAttr symName = getInnerSymName(op);
auto innerSym = lowerInnerSymbol(op);
auto name = op.getNameAttr();
if (AnnotationSet::removeAnnotations(op, dontTouchAnnoClass) && !symName) {
auto moduleName = cast<hw::HWModuleOp>(op->getParentOp()).getName();
// Prepend the name of the module to make the symbol name unique in the
// symbol table, it is already unique in the module. Checking if the name
// is unique in the SymbolTable is non-trivial.
symName = builder.getStringAttr(moduleNamespace.newName(
Twine("__") + moduleName + Twine("__") + name.getValue()));
}
// For now, if forceable ensure has symbol.
if (!symName && (!op.hasDroppableName() || op.isForceable())) {
auto moduleName = cast<hw::HWModuleOp>(op->getParentOp()).getName();
symName = builder.getStringAttr(moduleNamespace.newName(
Twine("__") + moduleName + Twine("__") + name.getValue()));
}
// This is not a temporary wire created by the compiler, so attach a symbol
// name.
auto wire = builder.create<hw::WireOp>(
op.getLoc(), getOrCreateZConstant(resultType), name, symName);
op.getLoc(), getOrCreateZConstant(resultType), name, innerSym);

if (auto svAttrs = sv::getSVAttributes(op))
sv::setSVAttributes(wire, svAttrs);
Expand Down Expand Up @@ -2828,27 +2841,15 @@ LogicalResult FIRRTLLowering::visitDecl(NodeOp op) {
// Node operations are logical noops, but may carry annotations or be
// referred to through an inner name. If a don't touch is present, ensure
// that we have a symbol name so we can keep the node as a wire.
auto symName = getInnerSymName(op);
auto name = op.getNameAttr();
if (AnnotationSet::removeAnnotations(op, dontTouchAnnoClass) && !symName) {
// name may be empty
auto moduleName = cast<hw::HWModuleOp>(op->getParentOp()).getName();
symName = builder.getStringAttr(Twine("__") + moduleName + Twine("__") +
name.getValue());
}
// For now, if forceable ensure has symbol.
if (!symName && (!hasDroppableName(op) || op.isForceable())) {
auto moduleName = cast<hw::HWModuleOp>(op->getParentOp()).getName();
symName = builder.getStringAttr(Twine("__") + moduleName + Twine("__") +
name.getValue());
}
auto innerSym = lowerInnerSymbol(op);

if (symName)
operand = builder.create<hw::WireOp>(operand, name, symName);
if (innerSym)
operand = builder.create<hw::WireOp>(operand, name, innerSym);

// Move SV attributes.
if (auto svAttrs = sv::getSVAttributes(op)) {
if (!symName)
if (!innerSym)
operand = builder.create<hw::WireOp>(operand, name);
sv::setSVAttributes(operand.getDefiningOp(), svAttrs);
}
Expand All @@ -2867,15 +2868,8 @@ LogicalResult FIRRTLLowering::visitDecl(RegOp op) {
if (!clockVal)
return failure();

// Add symbol if DontTouch annotation present.
// For now, also ensure has symbol if forceable.
auto innerSym = op.getInnerSymAttr();
if ((AnnotationSet::removeAnnotations(op, dontTouchAnnoClass) ||
op.getNameKind() == NameKindEnum::InterestingName || op.isForceable()) &&
!innerSym)
innerSym = hw::InnerSymAttr::get(op.getNameAttr());

// Create a reg op, wiring itself to its input.
auto innerSym = lowerInnerSymbol(op);
Backedge inputEdge = backedgeBuilder.get(resultType);
auto reg = builder.create<seq::FirRegOp>(inputEdge, clockVal,
op.getNameAttr(), innerSym);
Expand Down Expand Up @@ -2914,15 +2908,8 @@ LogicalResult FIRRTLLowering::visitDecl(RegResetOp op) {
if (!clockVal || !resetSignal || !resetValue)
return failure();

// Add symbol if DontTouch annotation present.
// For now, also ensure has symbol if forceable.
auto innerSym = op.getInnerSymAttr();
if ((AnnotationSet::removeAnnotations(op, dontTouchAnnoClass) ||
op.getNameKind() == NameKindEnum::InterestingName || op.isForceable()) &&
!innerSym)
innerSym = hw::InnerSymAttr::get(op.getNameAttr());

// Create a reg op, wiring itself to its input.
auto innerSym = lowerInnerSymbol(op);
bool isAsync = type_isa<AsyncResetType>(op.getResetSignal().getType());
Backedge inputEdge = backedgeBuilder.get(resultType);
auto reg =
Expand Down Expand Up @@ -3855,10 +3842,11 @@ Value FIRRTLLowering::createValueWithMuxAnnotation(Operation *op, bool isMux2) {
builder.setInsertionPoint(op);
StringRef namehint = isMux2 ? "mux2cell_in" : "mux4cell_in";
for (auto [idx, operand] : llvm::enumerate(op->getOperands())) {
auto sym = moduleNamespace.newName(Twine("__") + theModule.getName() +
Twine("__MUX__PRAGMA"));
auto [innerSym, _] = getOrAddInnerSym(
op->getContext(), /*attr=*/nullptr, 0,
[&]() -> hw::InnerSymbolNamespace & { return moduleNamespace; });
auto wire =
builder.create<hw::WireOp>(operand, namehint + Twine(idx), sym);
builder.create<hw::WireOp>(operand, namehint + Twine(idx), innerSym);
op->setOperand(idx, wire);
}
}
Expand Down
75 changes: 43 additions & 32 deletions lib/Dialect/FIRRTL/FIRRTLUtils.cpp
View file
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/HW/HWOps.h"
#include "circt/Dialect/HW/InnerSymbolNamespace.h"
#include "mlir/IR/ImplicitLocOpBuilder.h"
#include "llvm/ADT/TypeSwitch.h"
Expand Down Expand Up @@ -706,52 +707,48 @@ void circt::firrtl::walkGroundTypes(
recurse(recurse, type);
}

/// Returns an operation's `inner_sym`, adding one if necessary.
StringAttr circt::firrtl::getOrAddInnerSym(const hw::InnerSymTarget &target,
GetNamespaceCallback getNamespace) {

// Return InnerSymAttr with sym on specified fieldID.
auto getOrAdd = [&](auto mod, hw::InnerSymAttr attr,
auto fieldID) -> std::pair<hw::InnerSymAttr, StringAttr> {
assert(mod);
auto *context = mod.getContext();

SmallVector<hw::InnerSymPropertiesAttr> props;
if (attr) {
// If already present, return it.
if (auto sym = attr.getSymIfExists(fieldID))
return {attr, sym};
llvm::append_range(props, attr.getProps());
}
// Return InnerSymAttr with sym on specified fieldID.
std::pair<hw::InnerSymAttr, StringAttr> circt::firrtl::getOrAddInnerSym(
MLIRContext *context, hw::InnerSymAttr attr, uint64_t fieldID,
llvm::function_ref<hw::InnerSymbolNamespace &()> getNamespace) {
SmallVector<hw::InnerSymPropertiesAttr> props;
if (attr) {
// If already present, return it.
if (auto sym = attr.getSymIfExists(fieldID))
return {attr, sym};
llvm::append_range(props, attr.getProps());
}

// Otherwise, create symbol and add to list.
auto sym = StringAttr::get(context, getNamespace(mod).newName("sym"));
props.push_back(hw::InnerSymPropertiesAttr::get(
context, sym, fieldID, StringAttr::get(context, "public")));
// TODO: store/ensure always sorted, insert directly, faster search.
// For now, just be good and sort by fieldID.
llvm::sort(props, [](auto &p, auto &q) {
return p.getFieldID() < q.getFieldID();
});
return {hw::InnerSymAttr::get(context, props), sym};
};
// Otherwise, create symbol and add to list.
auto sym = StringAttr::get(context, getNamespace().newName("sym"));
props.push_back(hw::InnerSymPropertiesAttr::get(
context, sym, fieldID, StringAttr::get(context, "public")));
// TODO: store/ensure always sorted, insert directly, faster search.
// For now, just be good and sort by fieldID.
llvm::sort(props,
[](auto &p, auto &q) { return p.getFieldID() < q.getFieldID(); });
return {hw::InnerSymAttr::get(context, props), sym};
}

StringAttr circt::firrtl::getOrAddInnerSym(
const hw::InnerSymTarget &target,
llvm::function_ref<hw::InnerSymbolNamespace &()> getNamespace) {
if (target.isPort()) {
if (auto mod = dyn_cast<FModuleOp>(target.getOp())) {
auto portIdx = target.getPort();
assert(portIdx < mod.getNumPorts());
auto [attr, sym] =
getOrAdd(mod, mod.getPortSymbolAttr(portIdx), target.getField());
getOrAddInnerSym(mod.getContext(), mod.getPortSymbolAttr(portIdx),
target.getField(), getNamespace);
mod.setPortSymbolsAttr(portIdx, attr);
return sym;
}
} else {
// InnerSymbols only supported if op implements the interface.
if (auto symOp = dyn_cast<hw::InnerSymbolOpInterface>(target.getOp())) {
auto mod = symOp->getParentOfType<FModuleOp>();
assert(mod);
auto [attr, sym] =
getOrAdd(mod, symOp.getInnerSymAttr(), target.getField());
getOrAddInnerSym(symOp.getContext(), symOp.getInnerSymAttr(),
target.getField(), getNamespace);
symOp.setInnerSymbolAttr(attr);
return sym;
}
Expand All @@ -761,6 +758,20 @@ StringAttr circt::firrtl::getOrAddInnerSym(const hw::InnerSymTarget &target,
return {};
}

StringAttr circt::firrtl::getOrAddInnerSym(const hw::InnerSymTarget &target,
GetNamespaceCallback getNamespace) {
FModuleLike module;
if (target.isOpOnly())
module = target.getOp()->getParentOfType<FModuleOp>();
else
module = cast<FModuleLike>(target.getOp());
assert(module);

return getOrAddInnerSym(target, [&]() -> hw::InnerSymbolNamespace & {
return getNamespace(module);
});
}

/// Obtain an inner reference to an operation, possibly adding an `inner_sym`
/// to that operation.
hw::InnerRefAttr
Expand Down
Loading

0 comments on commit 56e98be

Please sign in to comment.