Skip to content

Commit

Permalink
[AddSeqMemPorts] Add hierpathop to verbatim, instead of raw instance…
Browse files Browse the repository at this point in the history 
… path (#7014)

The annotation `AddSeqMemPortsFileAnnotation` causes the pass
 `AddSeqMemPort` to add verbatim ops for the SRAM metadata file.
The file lists each SRAM and provides the mapping to
where it is in the hierarchy, and gives its IO prefix at the DUT top level.

This PR updates the pass to use `HierPathOp` to print the SRAM instance path,
 instead of the raw list of symbol references. 
This is required to ensure that the instance path is valid, even if the hierarchy
is updated by following passes.

This fixes a `firtool` crash that is exposed when using `AddSeqMem` along with
 `ExtractSeqMem`. Which is due to invalid symbol references in the `varbatim`, after
 the hierarchy was updated. Even though, this particular use case is invalid, as these
 two SRAM annotations cannot be used together, it can still be triggered by other
transformations.

Note: This metadata will soon be moved to OM, in a followup PR.
  • Loading branch information
@prithayan
prithayan committed May 10, 2024
1 parent 6a2b628 commit bc5ef52
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 20 deletions.
51 changes: 38 additions & 13 deletions lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
#include "PassDetails.h"
#include "circt/Dialect/Emit/EmitOps.h"
#include "circt/Dialect/FIRRTL/AnnotationDetails.h"
#include "circt/Dialect/FIRRTL/FIRRTLAnnotationHelper.h"
#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
#include "circt/Dialect/FIRRTL/FIRRTLOps.h"
#include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
#include "circt/Dialect/FIRRTL/Namespace.h"
#include "circt/Dialect/FIRRTL/Passes.h"
#include "circt/Dialect/HW/InnerSymbolNamespace.h"
#include "circt/Dialect/SV/SVOps.h"
Expand Down Expand Up @@ -66,8 +68,11 @@ struct AddSeqMemPortsPass : public AddSeqMemPortsBase<AddSeqMemPortsPass> {
/// which is for some reason the format of the metadata file.
std::vector<SmallVector<Attribute>> instancePaths;
};

CircuitNamespace circtNamespace;
/// This maps a module to information about the memories.
DenseMap<Operation *, MemoryInfo> memInfoMap;
DenseMap<Attribute, Operation *> innerRefToInstanceMap;

InstanceGraph *instanceGraph;

Expand Down Expand Up @@ -248,12 +253,13 @@ LogicalResult AddSeqMemPortsPass::processModule(FModuleOp module, bool isDUT) {
// current module.
auto &instancePaths = memInfo.instancePaths;
auto ref = getInnerRefTo(inst);
innerRefToInstanceMap[ref] = inst;
// If its a mem module, this is the start of a path to the module.
if (isa<FMemModuleOp>(submodule))
instancePaths.push_back({ref});
// Copy any paths through the submodule to memories, adding the ref to
// the current instance.
for (auto subPath : subMemInfo.instancePaths) {
for (const auto &subPath : subMemInfo.instancePaths) {
instancePaths.push_back(subPath);
instancePaths.back().push_back(ref);
}
Expand Down Expand Up @@ -285,6 +291,9 @@ void AddSeqMemPortsPass::createOutputFile(igraph::ModuleOpInterface module) {
std::string buffer;
llvm::raw_string_ostream os(buffer);

SymbolTable &symTable = getAnalysis<SymbolTable>();
HierPathCache cache(circuit, symTable);

// The current parameter to the verbatim op.
unsigned paramIndex = 0;
// Parameters to the verbatim op.
Expand All @@ -308,22 +317,37 @@ void AddSeqMemPortsPass::createOutputFile(igraph::ModuleOpInterface module) {

// The current sram we are processing.
unsigned sramIndex = 0;
auto dutSymbol = FlatSymbolRefAttr::get(module.getModuleNameAttr());
auto &instancePaths = memInfoMap[module].instancePaths;
for (auto instancePath : instancePaths) {
os << sramIndex++ << " -> ";
addSymbol(dutSymbol);
// The instance path is reverse from the order we print it.
for (auto ref : llvm::reverse(instancePath)) {
os << ".";
addSymbol(ref);
}
os << "\n";
}
auto dutSymbol = FlatSymbolRefAttr::get(module.getModuleNameAttr());

// Put the information in a verbatim operation.
auto loc = builder.getUnknownLoc();
// Put the information in a verbatim operation.
builder.create<emit::FileOp>(loc, outputFile, [&] {
for (auto instancePath : instancePaths) {
// Note: Reverse instancepath to construct the NLA.
SmallVector<Attribute> path(llvm::reverse(instancePath));
os << sramIndex++ << " -> ";
addSymbol(dutSymbol);
os << ".";

auto nlaSymbol = cache.getRefFor(builder.getArrayAttr(path));
addSymbol(nlaSymbol);
NamedAttrList fields;
// There is no current client for the distinct attr, but it will be used
// by OM::path once the metadata is moved to OM, instead of the verbatim.
auto id = DistinctAttr::create(UnitAttr::get(builder.getContext()));
fields.append("id", id);
fields.append("class", builder.getStringAttr("circt.tracker"));
fields.append("circt.nonlocal", nlaSymbol);
// Now add the nonlocal annotation to the leaf instance.
auto *leafInstance = innerRefToInstanceMap[instancePath.front()];

AnnotationSet annos(leafInstance);
annos.addAnnotations(builder.getDictionaryAttr(fields));
annos.applyToOperation(leafInstance);

os << "\n";
}
builder.create<sv::VerbatimOp>(loc, buffer, ValueRange{},
builder.getArrayAttr(params));
});
Expand All @@ -334,6 +358,7 @@ void AddSeqMemPortsPass::runOnOperation() {
auto *context = &getContext();
auto circuit = getOperation();
instanceGraph = &getAnalysis<InstanceGraph>();
circtNamespace = CircuitNamespace(circuit);
// Clear the state.
userPorts.clear();
memInfoMap.clear();
Expand Down
24 changes: 17 additions & 7 deletions test/Dialect/FIRRTL/add-seqmem-ports.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,28 @@ firrtl.circuit "Complex" attributes {annotations = [
input = false,
width = 4
}]} {

// CHECK: hw.hierpath private @[[memNLA:.+]] [@DUT::@[[MWRITE_EXT:.+]]]
// CHECK: hw.hierpath private @[[memNLA_0:.+]] [@DUT::@[[CHILD:.+]], @Child::@[[CHILD_MWRITE_EXT:.+]]]
// CHECK: hw.hierpath private @[[memNLA_1:.+]] [@DUT::@[[MWRITE_EXT_0:.+]]]
firrtl.memmodule @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>) attributes {dataWidth = 42 : ui32, depth = 12 : ui64, extraPorts = [], maskBits = 1 : ui32, numReadPorts = 0 : ui32, numReadWritePorts = 0 : ui32, numWritePorts = 1 : ui32, readLatency = 1 : ui32, writeLatency = 1 : ui32}
firrtl.module @Child() {
// CHECK: firrtl.instance MWrite_ext sym @[[CHILD_MWRITE_EXT:.+]] @MWrite_ext
// CHECK: firrtl.instance MWrite_ext sym @[[CHILD_MWRITE_EXT]]
// CHECK-SAME: circt.nonlocal = @[[memNLA_0]]
// CHECK-SAME: @MWrite_ext
%0:4 = firrtl.instance MWrite_ext @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>)
}
firrtl.module @DUT() attributes {annotations = [{class = "sifive.enterprise.firrtl.MarkDUTAnnotation"}]} {
// Double check that these instances now have symbols on them:
// CHECK: firrtl.instance MWrite_ext sym @[[MWRITE_EXT:.+]] @MWrite_ext(
// CHECK: firrtl.instance MWrite_ext sym @[[MWRITE_EXT]]
// CHECK-SAME: circt.nonlocal = @[[memNLA]]
// CHECK-SAME: @MWrite_ext(
%0:4 = firrtl.instance MWrite_ext @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>)
// CHECK: firrtl.instance child sym @[[CHILD:.+]] @Child(
// CHECK: firrtl.instance child sym @[[CHILD]] @Child(
firrtl.instance child @Child()
// CHECK: firrtl.instance MWrite_ext sym @[[MWRITE_EXT_0:.+]] @MWrite_ext(
// CHECK: firrtl.instance MWrite_ext sym @[[MWRITE_EXT_0]]
// CHECK-SAME: circt.nonlocal = @[[memNLA_1]]
// CHECK-SAME: @MWrite_ext(
%1:4 = firrtl.instance MWrite_ext @MWrite_ext(in W0_addr: !firrtl.uint<4>, in W0_en: !firrtl.uint<1>, in W0_clk: !firrtl.clock, in W0_data: !firrtl.uint<42>)
}
firrtl.module @Complex() {
Expand All @@ -176,8 +186,8 @@ firrtl.circuit "Complex" attributes {annotations = [
// CHECK: emit.file "metadata{{/|\\\\}}sram.txt" {
// CHECK-NEXT: sv.verbatim
// CHECK-SAME{LITERAL}: 0 -> {{0}}.{{1}}
// CHECK-SAME{LITERAL}: 1 -> {{0}}.{{2}}.{{3}}
// CHECK-SAME{LITERAL}: 2 -> {{0}}.{{4}}
// CHECK-SAME: {symbols = [@DUT, #hw.innerNameRef<@DUT::@[[MWRITE_EXT]]>, #hw.innerNameRef<@DUT::@[[CHILD]]>, #hw.innerNameRef<@Child::@[[CHILD_MWRITE_EXT]]>, #hw.innerNameRef<@DUT::@[[MWRITE_EXT_0]]>]}
// CHECK-SAME{LITERAL}: 1 -> {{0}}.{{2}}
// CHECK-SAME{LITERAL}: 2 -> {{0}}.{{3}}
// CHECK-SAME: {symbols = [@DUT, @[[memNLA]], @[[memNLA_0]], @[[memNLA_1]]]}
// CHECK-NEXT: }
}

0 comments on commit bc5ef52

Please sign in to comment.