[HW] Select the better name when dropping wires (#6559)

include/circt/Dialect/FIRRTL/FIRRTLOps.h

@@ -35,9 +35,6 @@ namespace firrtl {
 
 class StrictConnectOp;
 
-// is the name useless?
-bool isUselessName(circt::StringRef name);
-
 // works for regs, nodes, and wires
 bool hasDroppableName(Operation *op);
 

include/circt/Support/Naming.h

@@ -0,0 +1,32 @@
+//===- Naming.h - Utilities for handling names ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CIRCT_SUPPORT_NAMING_H
+#define CIRCT_SUPPORT_NAMING_H
+
+#include "circt/Support/LLVM.h"
+
+namespace circt {
+
+/// Return true if this is a possibly useless temporary name.
+/// This method is FIRRTL-centric, dropping useless temporaries.
+bool isUselessName(StringRef name);
+
+/// Choose a good name for an item from two options.
+StringRef chooseName(StringRef a, StringRef b);
+
+/// Choose a good name for an item from two options.
+StringAttr chooseName(StringAttr a, StringAttr b);
+
+/// Choose the better name between two ops. Picks the "name" attribute as first
+/// preference, using "sv.namehint" as an alternative.
+StringAttr chooseName(Operation *a, Operation *b);
+
+} // namespace circt
+
+#endif // CIRCT_SUPPORT_NAMING_H

lib/Conversion/SeqToSV/SeqToSV.cpp

@@ -22,6 +22,7 @@
 #include "circt/Dialect/SV/SVAttributes.h"
 #include "circt/Dialect/SV/SVOps.h"
 #include "circt/Dialect/Seq/SeqOps.h"
+#include "circt/Support/Naming.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/DialectImplementation.h"
 #include "mlir/IR/ImplicitLocOpBuilder.h"
@@ -261,6 +262,12 @@ class ClockCastLowering : public OpConversionPattern<T> {
   LogicalResult
   matchAndRewrite(T op, typename T::Adaptor adaptor,
                   ConversionPatternRewriter &rewriter) const final {
+    // If the cast had a better name than its input, propagate it.
+    if (Operation *inputOp = adaptor.getInput().getDefiningOp())
+      if (auto name = chooseName(op, inputOp))
+        rewriter.updateRootInPlace(
+            inputOp, [&] { inputOp->setAttr("sv.namehint", name); });
+
     rewriter.replaceOp(op, adaptor.getInput());
     return success();
   }

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

@@ -16,6 +16,7 @@
 #include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
 #include "circt/Support/APInt.h"
 #include "circt/Support/LLVM.h"
+#include "circt/Support/Naming.h"
 #include "mlir/IR/Matchers.h"
 #include "mlir/IR/PatternMatch.h"
 #include "llvm/ADT/APSInt.h"
@@ -85,32 +86,6 @@ static bool isUInt1(Type type) {
   return true;
 }
 
-// Heuristic to pick the best name.
-// Good names are not useless, don't start with an underscore, minimize
-// underscores in them, and are short. This function deterministically favors
-// the second name on ties.
-static StringRef chooseName(StringRef a, StringRef b) {
-  if (a.empty())
-    return b;
-  if (b.empty())
-    return a;
-  if (isUselessName(a))
-    return b;
-  if (isUselessName(b))
-    return a;
-  if (a.starts_with("_"))
-    return b;
-  if (b.starts_with("_"))
-    return a;
-  if (b.count('_') < a.count('_'))
-    return b;
-  if (b.count('_') > a.count('_'))
-    return a;
-  if (a.size() > b.size())
-    return b;
-  return a;
-}
-
 /// Set the name of an op based on the best of two names:  The current name, and
 /// the name passed in.
 static void updateName(PatternRewriter &rewriter, Operation *op,
@@ -155,15 +130,6 @@ static OpTy replaceOpWithNewOpAndCopyName(PatternRewriter &rewriter,
   return newOp;
 }
 
-/// Return true if this is a useless temporary name produced by FIRRTL.  We
-/// drop these as they don't convey semantic meaning.
-bool circt::firrtl::isUselessName(StringRef name) {
-  if (name.empty())
-    return true;
-  // Ignore _.*
-  return name.starts_with("_T") || name.starts_with("_WIRE");
-}
-
 /// Return true if the name is droppable. Note that this is different from
 /// `isUselessName` because non-useless names may be also droppable.
 bool circt::firrtl::hasDroppableName(Operation *op) {

lib/Dialect/FIRRTL/FIRRTLUtils.cpp

@@ -14,6 +14,7 @@
 #include "circt/Dialect/HW/HWOps.h"
 #include "circt/Dialect/HW/InnerSymbolNamespace.h"
 #include "circt/Dialect/Seq/SeqTypes.h"
+#include "circt/Support/Naming.h"
 #include "mlir/IR/ImplicitLocOpBuilder.h"
 #include "llvm/ADT/TypeSwitch.h"
 

lib/Dialect/FIRRTL/Transforms/DropName.cpp

@@ -14,6 +14,7 @@
 #include "circt/Dialect/FIRRTL/FIRRTLOps.h"
 #include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
 #include "circt/Dialect/FIRRTL/Passes.h"
+#include "circt/Support/Naming.h"
 
 using namespace circt;
 using namespace firrtl;

lib/Dialect/FIRRTL/Transforms/MaterializeDebugInfo.cpp

@@ -10,6 +10,7 @@
 #include "circt/Dialect/Debug/DebugOps.h"
 #include "circt/Dialect/FIRRTL/FIRRTLOps.h"
 #include "circt/Dialect/FIRRTL/FIRRTLTypes.h"
+#include "circt/Support/Naming.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/Builders.h"
 #include "llvm/ADT/StringExtras.h"

lib/Dialect/HW/HWOps.cpp

@@ -20,6 +20,7 @@
 #include "circt/Dialect/HW/ModuleImplementation.h"
 #include "circt/Support/CustomDirectiveImpl.h"
 #include "circt/Support/Namespace.h"
+#include "circt/Support/Naming.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/Interfaces/FunctionImplementation.h"
@@ -380,14 +381,10 @@ LogicalResult WireOp::canonicalize(WireOp wire, PatternRewriter &rewriter) {
 
   // If the wire has a name or an `sv.namehint` attribute, propagate it as an
   // `sv.namehint` to the expression.
-  if (auto *inputOp = wire.getInput().getDefiningOp()) {
-    auto name = wire.getNameAttr();
-    if (!name || name.getValue().empty())
-      name = wire->getAttrOfType<StringAttr>("sv.namehint");
-    if (name)
+  if (auto *inputOp = wire.getInput().getDefiningOp())
+    if (auto name = chooseName(wire, inputOp))
       rewriter.updateRootInPlace(
           inputOp, [&] { inputOp->setAttr("sv.namehint", name); });
-  }
 
   rewriter.replaceOp(wire, wire.getInput());
   return success();

lib/Support/CMakeLists.txt

@@ -13,6 +13,7 @@ add_circt_library(CIRCTSupport
   FieldRef.cpp
   JSON.cpp
   LoweringOptions.cpp
+  Naming.cpp
   Passes.cpp
   Path.cpp
   PrettyPrinter.cpp

lib/Support/Naming.cpp

@@ -0,0 +1,68 @@
+//===- Naming.cpp - Utilities for handling names ----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "circt/Support/Naming.h"
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/Operation.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace circt;
+
+bool circt::isUselessName(StringRef name) {
+  if (name.empty())
+    return true;
+  // Ignore _.*
+  return name.starts_with("_T") || name.starts_with("_WIRE");
+}
+
+// Heuristic to pick the best name.
+// Good names are not useless, don't start with an underscore, minimize
+// underscores in them, and are short. This function deterministically favors
+// the second name on ties.
+static bool isNameBetter(StringRef a, StringRef b) {
+  if (a.empty())
+    return false;
+  if (b.empty())
+    return true;
+  if (isUselessName(a))
+    return false;
+  if (isUselessName(b))
+    return true;
+  if (a.starts_with("_"))
+    return false;
+  if (b.starts_with("_"))
+    return true;
+  if (b.count('_') < a.count('_'))
+    return false;
+  if (b.count('_') > a.count('_'))
+    return true;
+  return a.size() <= b.size();
+}
+
+StringRef circt::chooseName(StringRef a, StringRef b) {
+  return isNameBetter(a, b) ? a : b;
+}
+
+StringAttr circt::chooseName(StringAttr a, StringAttr b) {
+  if (!a)
+    return b;
+  if (!b)
+    return a;
+  return isNameBetter(a.getValue(), b.getValue()) ? a : b;
+}
+
+static StringAttr getNameOrHint(Operation *a) {
+  StringAttr name = a->getAttrOfType<StringAttr>("name");
+  if (!name || name.getValue().empty())
+    return a->getAttrOfType<StringAttr>("sv.namehint");
+  return name;
+}
+
+StringAttr circt::chooseName(Operation *a, Operation *b) {
+  return chooseName(getNameOrHint(a), getNameOrHint(b));
+}

test/Dialect/HW/canonicalization.mlir

@@ -1741,18 +1741,23 @@ hw.module @Wires(in %a: i42) {
   // Wires should push their name or name hint onto their input when folding.
   %2 = comb.mul %a, %a : i42
   %3 = comb.mul %a, %a : i42
-  %4 = comb.mul %a, %a : i42
+  %4 = comb.mul %a, %a {sv.namehint = "preserve"} : i42
+  %5 = comb.mul %a, %a : i42
   %someName1 = hw.wire %2 : i42
-  %5 = hw.wire %3 {sv.namehint = "someName2"} : i42
-  %someName3 = hw.wire %4 {sv.namehint = "ignoredName"} : i42
+  %6 = hw.wire %3 {sv.namehint = "someName2"} : i42
+  %7 = hw.wire %4 {sv.namehint = "_ignored"} : i42
+  %someName3 = hw.wire %5 {sv.namehint = "someName3"} : i42
   hw.instance "names1" @WiresKeep(keep: %someName1: i42) -> ()
-  hw.instance "names2" @WiresKeep(keep: %5: i42) -> ()
-  hw.instance "names3" @WiresKeep(keep: %someName3: i42) -> ()
+  hw.instance "names2" @WiresKeep(keep: %6: i42) -> ()
+  hw.instance "names3" @WiresKeep(keep: %7: i42) -> ()
+  hw.instance "names4" @WiresKeep(keep: %someName3: i42) -> ()
   // CHECK-NEXT: %2 = comb.mul %a, %a {sv.namehint = "someName1"}
   // CHECK-NEXT: %3 = comb.mul %a, %a {sv.namehint = "someName2"}
-  // CHECK-NEXT: %4 = comb.mul %a, %a {sv.namehint = "someName3"}
+  // CHECK-NEXT: %4 = comb.mul %a, %a {sv.namehint = "preserve"}
+  // CHECK-NEXT: %5 = comb.mul %a, %a {sv.namehint = "someName3"}
   // CHECK-NEXT: hw.instance "names1" @WiresKeep(keep: %2: i42)
   // CHECK-NEXT: hw.instance "names2" @WiresKeep(keep: %3: i42)
   // CHECK-NEXT: hw.instance "names3" @WiresKeep(keep: %4: i42)
+  // CHECK-NEXT: hw.instance "names4" @WiresKeep(keep: %5: i42)
 }
 hw.module.extern @WiresKeep(in %keep: i42)