Remove includeHierarchy from convention attr. Clean up.

Author: uenoku

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

@@ -323,12 +323,11 @@ Example:
 
 ### Convention
 
-| Property         | Type   | Description                                          |
-| ---------------- | ------ | ---------------------------------------------------- |
-| class            | string | `circt.ConventionAnnotation`                         |
-| convention       | string | `scalarized`                                         |
-| target           | string | Reference target                                     |
-| includeHierarchy | bool   | Apply the convention to all modules in the hierarchy |
+| Property   | Type   | Description                             |
+| ---------- | ------ | --------------------------------------- |
+| class      | string | `circt.ConventionAnnotation`            |
+| convention | string | `scalarized`                            |
+| target     | string | Reference target                        |
 
 Specify the port convention for a module. The port convention controls how a
 module's ports are transformed, and how that module can be instantiated, in the
@@ -338,35 +337,33 @@ The options are:
 - `scalarized`: Convert aggregate ports (i.e. vector or bundles) into multiple
   ground-typed ports.
 
-`includeHierarchy` is optional and defaults to `false`, meaning that the
-convention is applied only to the specified module. If `includeHierarchy` is
-`true`, the convention is applied to all modules in the hierarchy. If there are
-multiple annotation instances that specify conventions, the `scalarized` convention
-takes precedence over the `internal` convention.
-
 ```json
 {
   "class": "circt.ConventionAnnotation",
   "convention": "scalarized",
-  "target": "~Foo|Bar",
-  "includeHierarchy": true
+  "target": "~Foo|Bar/d:Baz"
 }
 ```
 
 ### BodyTypeLoweringAnnotation
 
-| Property         | Type   | Description                        |
-| ---------------- | ------ | ---------------------------------- |
-| class            | string | `circt.BodyTypeLoweringAnnotation` |
-| convention       | string | See `Convention` annotation        |
-| target           | string | See `Convention` annotation        |
-| includeHierarchy | bool   | See `Convention` annotation        |
+| Property            | Type   | Description                                          |
+| ------------------- | ------ | ---------------------------------------------------- |
+| class               | string | `circt.BodyTypeLoweringAnnotation`                   |
+| convention          | string | See `Convention` annotation                          |
+| target              | string | See `Convention` annotation                          |
+| includeHierarchy    | bool   | Apply the convention to all modules in the hierarchy |
 
 Specify the type lowering option for module internal signals.
 This is similar to the `Convention` annotation, but for internal signals
 rather than module ports. Refer to the `Convention` annotation for each
 property description.
 
+When `includeHierarchy` is `false`, it indicates the convention is applied only to
+the specified module. If `includeHierarchy` is `true`, the convention is applied to
+all modules in the hierarchy. If there are multiple annotation instances that specify
+conventions, the `scalarized` convention takes precedence over the `internal` convention.
+
 ```json
 {
   "class": "circt.BodyTypeLoweringAnnotation",

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

@@ -268,17 +268,14 @@ static std::optional<Convention> parseConvention(llvm::StringRef str) {
       .Default(std::nullopt);
 }
 
-template <bool IsConventionAnno>
-static LogicalResult
-applyConventionOrTypeLoweringAnno(const AnnoPathValue &target,
-                                  DictionaryAttr anno, ApplyState &state) {
+static LogicalResult applyConventionAnno(const AnnoPathValue &target,
+                                         DictionaryAttr anno,
+                                         ApplyState &state) {
   auto *op = target.ref.getOp();
   auto loc = op->getLoc();
   auto error = [&]() {
     auto diag = mlir::emitError(loc);
-    diag << (IsConventionAnno ? "circuit.ConventionAnnotation "
-                              : "circuit.TypeLoweringAnnotation ")
-         << " ";
+    diag << "circuit.ConventionAnnotation ";
     return diag;
   };
 
@@ -301,47 +298,85 @@ applyConventionOrTypeLoweringAnno(const AnnoPathValue &target,
 
   auto convention = *conventionOpt;
 
-  if (convention == Convention::Internal)
-    // Convention is internal by default so there is nothing to change
-    return success();
-
-  auto includeHierarchy = anno.getAs<BoolAttr>("includeHierarchy");
-  auto conventionAttr = ConventionAttr::get(op->getContext(), convention);
-  auto setConvention = [&](Operation *moduleOp) {
-    TypeSwitch<Operation *>(moduleOp)
-        .Case<FModuleOp, FExtModuleOp>([&](auto moduleOp) {
-          if (IsConventionAnno)
-            moduleOp.setConventionAttr(conventionAttr);
-          else
-            moduleOp->setDiscardableAttr("body_type_lowering", conventionAttr);
-        })
-        .Default([](auto) {});
-  };
-
   if (auto moduleOp = dyn_cast<FModuleOp>(op)) {
-    if (includeHierarchy && includeHierarchy.getValue()) {
-      // If includeHierarchy is true, update the convention for all modules in
-      // the hierarchy.
-      for (auto *node :
-           llvm::post_order(state.instancePathCache.instanceGraph[moduleOp])) {
-        if (node && isa<FModuleOp, FExtModuleOp>(*node->getModule()))
-          setConvention(node->getModule());
-      }
-    } else {
-      // Update the convention.
-      setConvention(moduleOp);
-    }
+    moduleOp.setConvention(convention);
     return success();
   }
 
   if (auto extModuleOp = dyn_cast<FExtModuleOp>(op)) {
-    setConvention(extModuleOp);
+    extModuleOp.setConvention(convention);
     return success();
   }
 
   return error() << "can only target to a module or extmodule";
 }
 
+static LogicalResult applyBodyTypeLoweringAnno(const AnnoPathValue &target,
+                                               DictionaryAttr anno,
+                                               ApplyState &state) {
+  auto *op = target.ref.getOp();
+  auto loc = op->getLoc();
+  auto error = [&]() {
+    auto diag = mlir::emitError(loc);
+    diag << typeLoweringAnnoClass;
+    return diag;
+  };
+
+  auto opTarget = dyn_cast<OpAnnoTarget>(target.ref);
+  if (!opTarget)
+    return error() << "must target a module object";
+
+  if (!target.isLocal())
+    return error() << "must be local";
+
+  auto moduleOp = dyn_cast<FModuleOp>(op);
+
+  if (!moduleOp)
+    return error() << "can only target to a module";
+
+  auto conventionStrAttr =
+      tryGetAs<StringAttr>(anno, anno, "convention", loc, conventionAnnoClass);
+
+  if (!conventionStrAttr)
+    return failure();
+
+  auto conventionStr = conventionStrAttr.getValue();
+  auto conventionOpt = parseConvention(conventionStr);
+  if (!conventionOpt)
+    return error() << "unknown convention " << conventionStr;
+
+  auto convention = *conventionOpt;
+
+  if (convention == Convention::Internal)
+    // Convention is internal by default so there is nothing to change
+    return success();
+
+  auto conventionAttr = ConventionAttr::get(op->getContext(), convention);
+
+  // `includeHierarchy` only valid in BodyTypeLowering.
+  bool includeHierarchy = false;
+  if (auto includeHierarchyAttr = tryGetAs<BoolAttr>(
+          anno, anno, "includeHierarchy", loc, conventionAnnoClass))
+    includeHierarchy = includeHierarchyAttr.getValue();
+
+  if (includeHierarchy) {
+    // If includeHierarchy is true, update the convention for all modules in
+    // the hierarchy.
+    for (auto *node :
+         llvm::post_order(state.instancePathCache.instanceGraph[moduleOp])) {
+      if (!node)
+        continue;
+      if (auto fmodule = dyn_cast<FModuleOp>(*node->getModule()))
+        fmodule->setAttr("body_type_lowering", conventionAttr);
+    }
+  } else {
+    // Update the convention.
+    moduleOp->setAttr("body_type_lowering", conventionAttr);
+  }
+
+  return success();
+}
+
 static LogicalResult applyModulePrefixAnno(const AnnoPathValue &target,
                                            DictionaryAttr anno,
                                            ApplyState &state) {
@@ -583,10 +618,8 @@ static llvm::StringMap<AnnoRecord> annotationRecords{{
     {memTapPortClass, {stdResolve, applyWithoutTarget<true>}},
     {memTapBlackboxClass, {stdResolve, applyWithoutTarget<true>}},
     // Miscellaneous Annotations
-    {conventionAnnoClass,
-     {stdResolve, applyConventionOrTypeLoweringAnno<true>}},
-    {typeLoweringAnnoClass,
-     {stdResolve, applyConventionOrTypeLoweringAnno<false>}},
+    {conventionAnnoClass, {stdResolve, applyConventionAnno}},
+    {typeLoweringAnnoClass, {stdResolve, applyBodyTypeLoweringAnno}},
     {dontTouchAnnoClass,
      {stdResolve, applyWithoutTarget<true, true, WireOp, NodeOp, RegOp,
                                      RegResetOp, InstanceOp, MemOp, CombMemOp,

test/Dialect/FIRRTL/annotations.mlir

@@ -735,7 +735,7 @@ firrtl.circuit "Test" attributes {rawAnnotations = [
 
 firrtl.circuit "Test" attributes {rawAnnotations =[
   {class = "circt.ConventionAnnotation", target = "~Test|Test", convention = "scalarized"},
-  {class = "circt.BodyTypeLoweringAnnotation", target = "~Test|Test", convention = "scalarized"}
+  {class = "circt.BodyTypeLoweringAnnotation", target = "~Test|Test", convention = "scalarized", includeHierarchy = false}
   ]} {
   // CHECK: attributes {body_type_lowering = #firrtl<convention scalarized>, convention = #firrtl<convention scalarized>}
   firrtl.module @Test() attributes {convention = #firrtl<convention internal>} {}
@@ -744,15 +744,15 @@ firrtl.circuit "Test" attributes {rawAnnotations =[
 // -----
 
 firrtl.circuit "Test" attributes {rawAnnotations = [
-    {class = "circt.ConventionAnnotation", target = "~Test|Test", convention = "scalarized", includeHierarchy = true},
+    {class = "circt.ConventionAnnotation", target = "~Test|Test", convention = "scalarized"},
     {class = "circt.BodyTypeLoweringAnnotation", target = "~Test|Test", convention = "scalarized", includeHierarchy = true}
   ]} {
   // CHECK: @Test() attributes {body_type_lowering = #firrtl<convention scalarized>, convention = #firrtl<convention scalarized>}
   firrtl.module @Test() attributes {convention = #firrtl<convention internal>} {
     firrtl.instance child @Child()
   }
 
-  // CHECK: @Child() attributes {body_type_lowering = #firrtl<convention scalarized>, convention = #firrtl<convention scalarized>}
+  // CHECK: @Child() attributes {body_type_lowering = #firrtl<convention scalarized>}
   firrtl.module @Child() attributes {convention = #firrtl<convention internal>} {}
 
   // CHECK: @Child2() {