Output directory control v2

Instead of using an explicit precedence declaration anno to help guide the
assignment of floating modules to output directories, use the directory
hierarchy itself.  So if a module is used under directory A/B and A/C, it will
be placed into directory A.

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

@@ -1478,46 +1478,6 @@ Example:
 }
 ```
 
-### OutputDirPrecedenceAnnotation
-
-| Property   | Type    | Description                             |
-| ---------- | ------- | --------------------------------------- |
-| class      | string  | `circt.OutputDirPrecedenceAnnotation`   |
-| name       | string  | The output directory                    |
-| parent     | string  | The parent output directory             |
-
-Specify the "parent" of an 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 under a set of 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 a module available at all its instantiation
-sites.
-
-When an output directory isn't explicitly declared, then its parent directory is
-implicitly the default output directory. To explicitly declare that a
-directory's parent is the default output directory, use an empty string as the
-parent.
-
-Example:
-```json
-{
-  "class": "circt.OutputDirPrecedenceAnnotation",
-  "name": "verification_extras",
-  "parent": "verification"
-}
-```
-
 ## Attributes in SV
 
 Some annotations transform into attributes consumed by non-FIRRTL passes.  This

include/circt/Dialect/FIRRTL/AnnotationDetails.h

@@ -80,8 +80,6 @@ constexpr const char *testBenchDirAnnoClass =
 constexpr const char *moduleHierAnnoClass =
     "sifive.enterprise.firrtl.ModuleHierarchyAnnotation";
 constexpr const char *outputDirAnnoClass = "circt.OutputDirAnnotation";
-constexpr const char *ouputDirPrecedenceAnnoClass =
-    "circt.OutputDirPrecedenceAnnotation";
 constexpr const char *testHarnessHierAnnoClass =
     "sifive.enterprise.firrtl.TestHarnessHierarchyAnnotation";
 constexpr const char *retimeModulesFileAnnoClass =

include/circt/Dialect/FIRRTL/Passes.td

@@ -913,26 +913,10 @@ def AssignOutputDirs : Pass<"firrtl-assign-output-dirs", "firrtl::CircuitOp"> {
   let description = [{
     While some modules are assigned output directories by the user, many modules
     "don't care" what output directory they are placed into. This pass uses the
-    instance graph to assign these modules to the "narrowest" output directory
-    possible. For example, if a module is only instantiated under the
-    verification directory, then this pass will sink that module into the
-    verification directory, too. If a module is instantiated under multiple
-    directories, then the module is assigned to the directory that is the "least
-    common ancestor" of all directories it is instantiated under.
-
-    The least common ancestor is chosen according to a precedence graph provided
-    by the user through a circuit level annotation called
-    `OutputDirPrecedenceAnnotation`. The default output directory is implicitly 
-    the common ancestor of all other output directories.
+    instance graph to assign these modules to the "deepest" output directory
+    possible.
   }];
   let constructor = "circt::firrtl::createAssignOutputDirsPass()";
-  let statistics = [
-    Statistic<
-      "numLcaComputations",
-      "num-lca-computations",
-      "Number of times the LCA of two output directories is computed"
-    >
-  ];
 }
 
 #endif // CIRCT_DIALECT_FIRRTL_PASSES_TD

include/circt/Dialect/HW/HWAttributes.td

@@ -89,7 +89,7 @@ def OutputFileAttr : AttrDef<HWDialect, "OutputFile"> {
     bool isDirectory();
 
     /// Get the directory of this output file, or null if there is none.
-    ::mlir::StringAttr getDirectoryAttr();
+    ::mlir::StringRef getDirectory();
   }];
 }
 

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

@@ -21,208 +21,48 @@
 
 using namespace circt;
 using namespace firrtl;
+namespace path = llvm::sys::path;
 
-//===----------------------------------------------------------------------===//
-// Directory Utilities
-//===----------------------------------------------------------------------===//
-
-static SmallString<128> canonicalize(StringRef directory) {
-  SmallString<128> native;
-  if (directory.empty())
-    return native;
+using hw::OutputFileAttr;
 
-  llvm::sys::path::native(directory, native);
-  auto separator = llvm::sys::path::get_separator();
-  if (!native.ends_with(separator))
-    native += separator;
-  return native;
-}
+static StringRef lca(StringRef a, OutputFileAttr file) {
+  if (!file)
+    return StringRef();
 
-static StringAttr canonicalize(const StringAttr directory) {
-  if (!directory)
-    return nullptr;
-  if (directory.empty())
-    return nullptr;
-  return StringAttr::get(directory.getContext(),
-                         canonicalize(directory.getValue()));
-}
+  auto b = file.getDirectory();
+  if (llvm::sys::path::is_absolute(b))
+    return StringRef();
 
-//===----------------------------------------------------------------------===//
-// Output Directory Priority Table
-//===----------------------------------------------------------------------===//
-
-namespace {
-
-struct OutputDirInfo {
-  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;
-};
+  for (auto i = path::begin(b), e = path::end(b); i != e; ++i)
+    if (*i == "..")
+      return StringRef();
 
-/// A table that helps decide which directory a floating module must be placed.
-/// Given two candidate output directories, the table can answer the question,
-/// which directory should a resource go.
-///
-/// Output directories are organized into a tree, which represents the relative
-/// "specificity" of a directory. If a resource could be placed in more than one
-/// directory, then it is output in the least-common-ancestor of the
-/// candidate output directories, which represents the "most specific" place
-/// a resource could go, which is still general enough to cover all uses.
-class OutputDirTable {
-public:
-  LogicalResult initialize(CircuitOp);
+  size_t i = 0;
+  size_t e = std::min(a.size(), b.size());
+  for (; i < e; ++i)
+    if (a[i] != b[i])
+      break;
 
-  /// 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 lca(StringAttr, StringAttr);
+  auto dir = a.substr(0, i);
+  if (dir.ends_with(llvm::sys::path::get_separator()))
+    return dir;
 
-  unsigned getNumLcaComputations() const { return numLcaComputations; }
-
-private:
-  DenseMap<StringAttr, size_t> indexTable;
-  std::vector<OutputDirInfo> infoTable;
-  unsigned numLcaComputations = 0;
-};
-} // namespace
-
-LogicalResult OutputDirTable::initialize(CircuitOp circuit) {
-  auto err = [&]() { return emitError(circuit.getLoc()); };
-
-  // Stage 1: Build a table mapping child directories to their parents.
-  indexTable[nullptr] = 0;
-  infoTable.emplace_back(nullptr, 0, SIZE_MAX);
-  AnnotationSet annos(circuit);
-  for (auto anno : annos) {
-    if (anno.isClass(ouputDirPrecedenceAnnoClass)) {
-      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: Set the depth/priority of each directory, and check for cycles.
-  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) {
-      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;
-      }
-      current->depth = parent->depth + 1;
-      if (stack.empty())
-        break;
-      i = stack.back();
-      stack.pop_back();
-    }
-  }
-  return success();
+  return llvm::sys::path::parent_path(dir);
 }
 
-StringAttr OutputDirTable::lca(StringAttr nameA, StringAttr nameB) {
-  if (!nameA || !nameB)
-    return nullptr;
-  if (nameA == nameB)
-    return nameA;
-
-  auto lookupA = indexTable.find(nameA);
-  if (lookupA == indexTable.end())
-    return nullptr;
-
-  auto lookupB = indexTable.find(nameB);
-  if (lookupB == indexTable.end())
-    return nullptr;
-
-  ++numLcaComputations;
-
-  auto a = infoTable[lookupA->second];
-  auto b = infoTable[lookupB->second];
-
-  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];
-  }
-  return a.name;
+static OutputFileAttr getOutputFile(Operation *op) {
+  return op->getAttrOfType<hw::OutputFileAttr>("output_file");
 }
 
-//===----------------------------------------------------------------------===//
-// Pass Infrastructure
-//===----------------------------------------------------------------------===//
-
 namespace {
 class AssignOutputDirsPass : public AssignOutputDirsBase<AssignOutputDirsPass> {
   void runOnOperation() override;
 };
 } // namespace
 
-static StringAttr getOutputDir(Operation *op) {
-  auto outputFile = op->getAttrOfType<hw::OutputFileAttr>("output_file");
-  if (!outputFile)
-    return nullptr;
-  return outputFile.getDirectoryAttr();
-}
-
 void AssignOutputDirsPass::runOnOperation() {
   auto falseAttr = BoolAttr::get(&getContext(), false);
-  auto circuit = getOperation();
-  OutputDirTable outDirTable;
-  if (failed(outDirTable.initialize(circuit)))
-    return signalPassFailure();
+  bool changed = false;
 
   DenseSet<InstanceGraphNode *> visited;
   for (auto *root : getAnalysis<InstanceGraph>()) {
@@ -231,31 +71,36 @@ void AssignOutputDirsPass::runOnOperation() {
       if (!module || module->getAttrOfType<hw::OutputFileAttr>("output_file") ||
           module.isPublic())
         continue;
-      StringAttr outputDir;
+      StringRef outputDir;
       auto i = node->usesBegin();
       auto e = node->usesEnd();
       for (; i != e; ++i) {
         if (auto parent = dyn_cast<FModuleOp>((*i)->getParent()->getModule())) {
-          outputDir = getOutputDir(parent);
+          auto file = getOutputFile(parent);
+          if (file)
+            outputDir = file.getDirectory();
           ++i;
           break;
         }
       }
       for (; i != e; ++i) {
-        if (outputDir == nullptr)
+        if (outputDir.empty())
           break;
         if (auto parent =
                 dyn_cast<FModuleOp>((*i)->getParent()->getModule<FModuleOp>()))
-          outputDir = outDirTable.lca(outputDir, getOutputDir(parent));
+          outputDir = lca(outputDir, getOutputFile(parent));
+      }
+      if (!outputDir.empty()) {
+        auto s = StringAttr::get(&getContext(), outputDir);
+        auto f = hw::OutputFileAttr::get(s, falseAttr, falseAttr);
+        module->setAttr("output_file", f);
+        changed = true;
       }
-      if (outputDir)
-        module->setAttr("output_file", hw::OutputFileAttr::get(
-                                           outputDir, falseAttr, falseAttr));
     }
   }
 
-  numLcaComputations = outDirTable.getNumLcaComputations();
-  markAllAnalysesPreserved();
+  if (!changed)
+    markAllAnalysesPreserved();
 }
 
 std::unique_ptr<mlir::Pass> circt::firrtl::createAssignOutputDirsPass() {

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

@@ -518,7 +518,6 @@ static llvm::StringMap<AnnoRecord> annotationRecords{{
     {metadataDirectoryAttrName, NoTargetAnnotation},
     {moduleHierAnnoClass, NoTargetAnnotation},
     {outputDirAnnoClass, {stdResolve, applyOutputDirAnno}},
-    {ouputDirPrecedenceAnnoClass, NoTargetAnnotation},
     {sitestTestHarnessBlackBoxAnnoClass, NoTargetAnnotation},
     {testBenchDirAnnoClass, NoTargetAnnotation},
     {testHarnessHierAnnoClass, NoTargetAnnotation},

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp

@@ -792,7 +792,7 @@ void LowerLayersPass::runOnOperation() {
     hw::OutputFileAttr bindFile;
     if (auto outputFile =
             layerOp->getAttrOfType<hw::OutputFileAttr>("output_file")) {
-      auto dir = outputFile.getDirectoryAttr().getValue();
+      auto dir = outputFile.getDirectory();
       bindFile = hw::OutputFileAttr::getFromDirectoryAndFilename(
           &getContext(), dir, prefix + ".sv",
           /*excludeFromFileList=*/true);