Cleanup

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

@@ -1484,18 +1484,23 @@ Example:
 
 Specify the "parent" of an output directory.
 
-When verilog is output, some modules will have user-specified output
-directories. If a private module is instantiated only under an output directory,
-then it can be "pulled in" to that directory, too.
-
-If a module is instantiated under multiple output directories, the module is
-placed in the output directory that is the least-common-ancestor of the module's
-uses. The least-common-ancestor is identified according to these output
-directory declaration annotations.
-
-The default output directory is implicitly the most general output directory.
-When a directory isn't explicitly declared, then the parent is presumed to be
-the default output directory.
+When Verilog is output, some modules will have user-specified output
+directories. When a module M is only instantiated by modules which have a common
+output directory D, we can also output module M in that same directory D.
+
+If the module M is instantiated by modules which have different output
+directories DS, then the module M is placed in the output directory that is the
+least-common-ancestor (LCA) of the directories DS. The LCA is identified
+according to this output directory declaration annotation.
+
+The intuition behind this annotation is that modules output in the declared
+directory should only depend on modules which are output in an ancestor
+directory. Then, the LCA can be thought of as the "most specific" output
+directory that still makes the module M available at all its instantiation
+sites.
+
+When an output directory isn't explicitly declared, then its parent directory is
+implicitly the default output directory.
 
 Example:
 ```json

lib/Dialect/FIRRTL/Import/FIRParser.cpp

@@ -5318,8 +5318,10 @@ ParseResult FIRCircuitParser::parseLayer(CircuitOp circuit) {
 
     hw::OutputFileAttr outputDir;
     if (getToken().getKind() == FIRToken::string) {
-      outputDir = hw::OutputFileAttr::getAsDirectory(
-          getContext(), getToken().getStringValue());
+      auto text = getToken().getStringValue();
+      if (text.empty())
+        return emitError() << "output directory must not be blank";
+      outputDir = hw::OutputFileAttr::getAsDirectory(getContext(), text);
       consumeToken(FIRToken::string);
     }
     if (parseToken(FIRToken::colon, "expected ':' after layer definition") ||

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

@@ -14,6 +14,7 @@
 #include "circt/Dialect/HW/HWAttributes.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PostOrderIterator.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/Path.h"
 
 #define DEBUG_TYPE "firrtl-assign-output-dirs"
@@ -25,8 +26,11 @@ using namespace firrtl;
 // Directory Utilities
 //===----------------------------------------------------------------------===//
 
-static SmallString<128> canonicalize(const Twine &directory) {
+static SmallString<128> canonicalize(StringRef directory) {
   SmallString<128> native;
+  if (directory.empty())
+    return native;
+
   llvm::sys::path::native(directory, native);
   auto separator = llvm::sys::path::get_separator();
   if (!native.ends_with(separator))
@@ -37,20 +41,25 @@ static SmallString<128> canonicalize(const Twine &directory) {
 static StringAttr canonicalize(const StringAttr directory) {
   if (!directory)
     return nullptr;
+  if (directory.empty())
+    return nullptr;
   return StringAttr::get(directory.getContext(),
-                         canonicalize(Twine(directory.getValue())));
+                         canonicalize(directory.getValue()));
 }
 
 //===----------------------------------------------------------------------===//
 // Output Directory Priority Table
 //===----------------------------------------------------------------------===//
 
 namespace {
+
 struct OutputDirInfo {
-  OutputDirInfo(unsigned depth, StringAttr parent)
-      : depth(depth), parent(parent) {}
-  unsigned depth;
-  StringAttr parent;
+  OutputDirInfo(StringAttr name, size_t depth = SIZE_MAX,
+                size_t parent = SIZE_MAX)
+      : name(name), depth(depth), parent(parent) {}
+  StringAttr name;
+  size_t depth;
+  size_t parent;
 };
 
 /// A table that helps decide which directory a floating module must be placed.
@@ -64,84 +73,125 @@ struct OutputDirInfo {
 /// a resource could go, which is still general enough to cover all uses.
 class OutputDirTable {
 public:
-  explicit OutputDirTable(CircuitOp);
+  LogicalResult initialize(CircuitOp);
 
   /// Given two directory names, returns the least-common-ancestor directory.
   /// If the LCA is the toplevel output directory (which is considered the most
   /// general), return null.
-  StringAttr join(StringAttr, StringAttr);
+  StringAttr lca(StringAttr, StringAttr);
 
 private:
-  OutputDirInfo get(StringAttr);
-
-  DenseMap<StringAttr, OutputDirInfo> info;
+  DenseMap<StringAttr, size_t> indexTable;
+  std::vector<OutputDirInfo> infoTable;
 };
 } // namespace
 
-OutputDirTable::OutputDirTable(CircuitOp circuit) {
+LogicalResult OutputDirTable::initialize(CircuitOp circuit) {
+  auto err = [&]() { return emitError(circuit.getLoc()); };
+
   // Stage 1: Build a table mapping child directories to their parents.
-  DenseMap<StringAttr, StringAttr> parentTable;
+  indexTable[nullptr] = 0;
+  infoTable.emplace_back(nullptr, 0, SIZE_MAX);
   AnnotationSet annos(circuit);
   for (auto anno : annos) {
     if (anno.isClass(declareOutputDirAnnoClass)) {
-      auto name = canonicalize(anno.getMember<StringAttr>("name"));
-      auto parent = canonicalize(anno.getMember<StringAttr>("parent"));
-      if (name)
-        parentTable[name] = parent;
+      auto nameField = anno.getMember<StringAttr>("name");
+      if (!nameField)
+        return err() << "output directory declaration missing name";
+      if (nameField.empty())
+        return err() << "output directory name cannot be empty";
+      auto name = canonicalize(nameField);
+
+      auto parentField = anno.getMember<StringAttr>("parent");
+      if (!parentField)
+        return err() << "output directory declaration missing parent";
+      auto parent = canonicalize(parentField);
+
+      auto parentIdx = infoTable.size();
+      {
+        auto [it, inserted] = indexTable.try_emplace(parent, parentIdx);
+        if (inserted)
+          infoTable.emplace_back(parent, SIZE_MAX, SIZE_MAX);
+        else
+          parentIdx = it->second;
+      }
+
+      {
+        auto [it, inserted] = indexTable.try_emplace(name, infoTable.size());
+        if (inserted) {
+          infoTable.emplace_back(name, SIZE_MAX, parentIdx);
+        } else {
+          auto &child = infoTable[it->second];
+          assert(child.name == name);
+          if (child.parent != SIZE_MAX)
+            return err() << "output directory " << name
+                         << " declared multiple times";
+          child.parent = parentIdx;
+        }
+      }
     }
   }
+  for (auto &info : infoTable)
+    if (info.parent == SIZE_MAX)
+      info.parent = 0;
 
   // Stage 2: Process the parentTable into a precedence graph.
-  info.insert({nullptr, {0, nullptr}});
-  SmallVector<std::pair<StringAttr, StringAttr>> stack;
-  for (auto [current, parent] : parentTable) {
-    auto it = info.find(current);
-    if (it != info.end())
+  SmallVector<size_t> stack;
+  BitVector seen(infoTable.size(), false);
+  for (unsigned i = 0, e = infoTable.size(); i < e; ++i) {
+    auto *current = &infoTable[i];
+    if (current->depth != SIZE_MAX)
       continue;
+    seen.reset();
+    seen.set(i);
     while (true) {
-      auto it = info.find(parent);
-      if (it == info.end()) {
-        stack.push_back({current, parent});
-        current = parent;
-        parent = parentTable.lookup(current);
+      seen.set(i);
+      auto *current = &infoTable[i];
+      auto *parent = &infoTable[current->parent];
+      if (seen[current->parent])
+        return emitError(circuit.getLoc())
+               << "circular precedence between output directories "
+               << current->name << " and " << parent->name;
+      if (parent->depth == SIZE_MAX) {
+        stack.push_back(i);
+        i = current->parent;
         continue;
       }
-      info.insert({current, {it->second.depth + 1, parent}});
+      current->depth = parent->depth + 1;
       if (stack.empty())
         break;
-      std::tie(current, parent) = stack.back();
+      i = stack.back();
       stack.pop_back();
     }
   }
+  return success();
 }
 
-OutputDirInfo OutputDirTable::get(StringAttr dir) {
-  return info.insert({dir, {1, nullptr}}).first->second;
-}
-
-StringAttr OutputDirTable::join(StringAttr a, StringAttr b) {
-  if (!a || !b)
+StringAttr OutputDirTable::lca(StringAttr nameA, StringAttr nameB) {
+  if (!nameA || !nameB)
     return nullptr;
-  if (a == b)
-    return a;
-  auto ainfo = get(a);
-  auto binfo = get(b);
-  while (ainfo.depth < binfo.depth) {
-    a = ainfo.parent;
-    ainfo = get(a);
+  if (nameA == nameB)
+    return nameA;
+
+  auto lookup = [&](StringAttr dir) -> OutputDirInfo {
+    auto it = indexTable.find(dir);
+    if (it != indexTable.end())
+      return infoTable[it->second];
+    return {dir, 1, 0};
+  };
+
+  auto a = lookup(nameA);
+  auto b = lookup(nameB);
+
+  while (a.depth > b.depth)
+    a = infoTable[a.parent];
+  while (b.depth > a.depth)
+    b = infoTable[b.parent];
+  while (a.name != b.name) {
+    a = infoTable[a.parent];
+    b = infoTable[b.parent];
   }
-  while (binfo.depth < ainfo.depth) {
-    b = binfo.parent;
-    binfo = get(b);
-  }
-  while (a != b) {
-    a = ainfo.parent;
-    b = binfo.parent;
-    ainfo = get(a);
-    binfo = get(b);
-  }
-  assert(a == b);
-  return a;
+  return a.name;
 }
 
 //===----------------------------------------------------------------------===//
@@ -164,7 +214,10 @@ static StringAttr getOutputDir(Operation *op) {
 void AssignOutputDirsPass::runOnOperation() {
   auto falseAttr = BoolAttr::get(&getContext(), false);
   auto circuit = getOperation();
-  OutputDirTable outDirTable(circuit);
+  OutputDirTable outDirTable;
+  if (failed(outDirTable.initialize(circuit)))
+    return signalPassFailure();
+
   DenseSet<InstanceGraphNode *> visited;
   for (auto *root : getAnalysis<InstanceGraph>()) {
     for (auto *node : llvm::inverse_post_order_ext(root, visited)) {
@@ -185,7 +238,7 @@ void AssignOutputDirsPass::runOnOperation() {
       for (; i != e; ++i) {
         if (auto parent =
                 dyn_cast<FModuleOp>((*i)->getParent()->getModule<FModuleOp>()))
-          outputDir = outDirTable.join(outputDir, getOutputDir(parent));
+          outputDir = outDirTable.lca(outputDir, getOutputDir(parent));
       }
       if (outputDir)
         module->setAttr("output_file", hw::OutputFileAttr::get(

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp

@@ -831,10 +831,10 @@ void LowerLayersPass::runOnOperation() {
     ::setOutputFile(footer, bindFile);
 
     if (!layerOp.getBody().getOps<LayerOp>().empty())
-      layers.push_back(
-          {layerOp,
-           builder.getStringAttr("`include \"" +
-                                 bindFile.getFilename().getValue() + "\"")});
+      layers.emplace_back(
+          layerOp,
+          builder.getStringAttr("`include \"" +
+                                bindFile.getFilename().getValue() + "\""));
   });
 
   // All layers definitions can now be deleted.

test/Dialect/FIRRTL/assign-output-dirs.mlir

@@ -1,44 +1,52 @@
 // RUN: circt-opt -firrtl-assign-output-dirs %s
 
-// ------
-
 firrtl.circuit "AssignOutputDirs"
   attributes {
       // Directory Precedence Tree
       //        R
       //    A       B
-      //  C   D   E   F
+      //  C   D
       annotations = [
+        {class = "circt.DeclareOutputDirAnnotation", name = "B", parent = ""},
         {class = "circt.DeclareOutputDirAnnotation", name = "C", parent = "A"},
-        {class = "circt.DeclareOutputDirAnnotation", name = "D", parent = "A"},
-        {class = "circt.DeclareOutputDirAnnotation", name = "E", parent = "B"},
-        {class = "circt.DeclareOutputDirAnnotation", name = "F", parent = "B"}
+        {class = "circt.DeclareOutputDirAnnotation", name = "D", parent = "A"}
       ]
   } {
 
   firrtl.module private @AssignOutputDirs() {}
 
   // R -> R
+  // CHECK: firrtl.module private @ByR() {
   firrtl.module private @ByR() {}
 
   // R & A -> R
+  // CHECK: firrtl.module private @ByRA() {
   firrtl.module private @ByRA() {}
 
   // R & C -> R
+  // CHECK: firrtl.module private @ByRC() {
   firrtl.module private @ByRC() {}
 
   // A -> A
+  // CHECK: firrtl.module private @ByA() attributes {output_file = #hw.output_file<"A/">} {
   firrtl.module private @ByA() {}
 
-  // A & B -> <null>
+  // A & B -> R
+  // firrtl.module private @ByAB() {
   firrtl.module private @ByAB() {}
 
   // C & D -> A
+  // CHECK: firrtl.module private @ByCD() attributes {output_file = #hw.output_file<"A/">} {
   firrtl.module private @ByCD() {}
 
   // A & C -> A
+  // CHECK: firrtl.module private @ByAC() attributes {output_file = #hw.output_file<"A/">} {
   firrtl.module private @ByAC() {}
 
+  // B & C -> R
+  // CHECK: firrtl.module private @ByBC() attributes {
+  firrtl.module private @ByBC() {}
+
   firrtl.module @InR() {
     firrtl.instance r  @ByR()
     firrtl.instance ra @ByRA()
@@ -54,10 +62,12 @@ firrtl.circuit "AssignOutputDirs"
 
   firrtl.module @InB() attributes {output_file = #hw.output_file<"B/foo">} {
     firrtl.instance ab @ByAB()
+    firrtl.instance bc @ByBC()
   }
 
   firrtl.module @InC() attributes {output_file = #hw.output_file<"C/">} {
-    firrtl.instance byCD @ByCD()
+    firrtl.instance cd @ByCD()
+    firrtl.instance bc @ByBC()
   }
 
   firrtl.module @InD() attributes {output_file = #hw.output_file<"D/">} {
@@ -72,4 +82,8 @@ firrtl.circuit "AssignOutputDirs"
 
 // when a module is used in both a child and parent, it is dragged
 
-// when a module is used by two children, it is dragged to parent.
+// when a module is used by two children, it is dragged to parent.
+
+// circular parent/child relationship
+// parent is null / root
+// child name is empty???