[FIRRTL] Remove convention verification for InstanceChoice (#10066)

* Remove verification that requires referred modules have same conventions in InstanceChoice
* Force scalarized convention in LowerTypes/LowerSignatures for referred modules of InstanceChoice

Author: uenoku

include/circt/Dialect/FIRRTL/FIRRTLUtils.h

@@ -13,6 +13,7 @@
 #ifndef CIRCT_DIALECT_FIRRTL_FIRRTLUTILS_H
 #define CIRCT_DIALECT_FIRRTL_FIRRTLUTILS_H
 
+#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
 #include "circt/Dialect/FIRRTL/FIRRTLOps.h"
 #include "mlir/IR/BuiltinOps.h"
 

lib/Dialect/FIRRTL/FIRRTLOps.cpp

@@ -3165,7 +3165,6 @@ LogicalResult InstanceChoiceOp::verify() {
 LogicalResult
 InstanceChoiceOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
   auto caseNames = getCaseNamesAttr();
-  std::optional<Convention> convention;
   for (auto moduleName : getModuleNamesAttr()) {
     auto moduleNameRef = cast<FlatSymbolRefAttr>(moduleName);
     if (failed(instance_like_impl::verifyReferencedModule(*this, symbolTable,
@@ -3178,14 +3177,6 @@ InstanceChoiceOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
     if (isa<FIntModuleOp>(referencedModule))
       return emitOpError("intmodule must be instantiated with instance op, "
                          "not via 'firrtl.instance_choice'");
-
-    if (!convention) {
-      convention = referencedModule.getConvention();
-      continue;
-    }
-
-    if (*convention != referencedModule.getConvention())
-      return emitOpError("all modules must have the same convention");
   }
 
   auto root = cast<SymbolRefAttr>(caseNames[0]).getRootReference();

lib/Dialect/FIRRTL/FIRRTLUtils.cpp

@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
+#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h"
 #include "circt/Dialect/HW/HWOps.h"
 #include "circt/Dialect/HW/InnerSymbolNamespace.h"
 #include "circt/Dialect/Seq/SeqTypes.h"

lib/Dialect/FIRRTL/Transforms/LowerSignatures.cpp

@@ -22,6 +22,7 @@
 #include "circt/Support/InstanceGraphInterface.h"
 #include "mlir/IR/Threading.h"
 #include "mlir/Pass/Pass.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "firrtl-lower-signatures"
@@ -508,6 +509,7 @@ struct LowerSignaturesPass
 // This is the main entrypoint for the lowering pass.
 void LowerSignaturesPass::runOnOperation() {
   CIRCT_DEBUG_SCOPED_PASS_LOGGER(this);
+  auto &instanceGraph = getAnalysis<InstanceGraph>();
 
   // Cached attr
   AttrCache cache(&getContext());
@@ -516,8 +518,15 @@ void LowerSignaturesPass::runOnOperation() {
   auto circuit = getOperation();
 
   for (auto mod : circuit.getOps<FModuleLike>()) {
-    if (lowerModuleSignature(mod, mod.getConvention(), cache,
-                             portMap[mod.getNameAttr()])
+    auto convention = mod.getConvention();
+    // Instance choices select between modules with a shared port shape, so
+    // any module instantiated by one must use the scalarized convention.
+    if (llvm::any_of(instanceGraph.lookup(mod)->uses(),
+                     [](InstanceRecord *use) {
+                       return use->getInstance<InstanceChoiceOp>();
+                     }))
+      convention = Convention::Scalarized;
+    if (lowerModuleSignature(mod, convention, cache, portMap[mod.getNameAttr()])
             .failed())
       return signalPassFailure();
   }

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

@@ -41,6 +41,7 @@
 #include "mlir/Pass/Pass.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "firrtl-lower-types"
@@ -1828,6 +1829,7 @@ void LowerTypesPass::runOnOperation() {
   CIRCT_DEBUG_SCOPED_PASS_LOGGER(this);
 
   std::vector<FModuleLike> ops;
+  auto &instanceGraph = getAnalysis<InstanceGraph>();
   // Symbol Table
   auto &symTbl = getAnalysis<SymbolTable>();
   // Cached attr
@@ -1836,7 +1838,15 @@ void LowerTypesPass::runOnOperation() {
   DenseMap<FModuleLike, Convention> conventionTable;
   auto circuit = getOperation();
   for (auto module : circuit.getOps<FModuleLike>()) {
-    conventionTable.insert({module, module.getConvention()});
+    auto convention = module.getConvention();
+    // Instance choices select between modules with a shared port shape, so
+    // any module instantiated by one must use the scalarized convention.
+    if (llvm::any_of(instanceGraph.lookup(module)->uses(),
+                     [](InstanceRecord *use) {
+                       return use->getInstance<InstanceChoiceOp>();
+                     }))
+      convention = Convention::Scalarized;
+    conventionTable.insert({module, convention});
     ops.push_back(module);
   }
 

test/Dialect/FIRRTL/errors.mlir

@@ -3485,27 +3485,3 @@ firrtl.circuit "DomainCreateWrongFieldType" {
     // expected-error @-1 {{use of value '%period' expects different type than prior uses: '!firrtl.integer' vs '!firrtl.string'}}
   }
 }
-
-// -----
-
-firrtl.circuit "InstanceChoiceAggregate" {
-  firrtl.option @Platform {
-    firrtl.option_case @FPGA
-  }
-  firrtl.module private @Target(
-    in %in: !firrtl.vector<uint<8>, 2>,
-    out %out: !firrtl.vector<uint<8>, 2>
-  ) attributes {convention = #firrtl<convention internal>} { }
-
-  firrtl.module public @PublicTarget(
-    in %in: !firrtl.vector<uint<8>, 2>,
-    out %out: !firrtl.vector<uint<8>, 2>
-  ) attributes {convention = #firrtl<convention scalarized>}{ }
-
-  firrtl.module @InstanceChoiceAggregate() {
-    // expected-error @below {{'firrtl.instance_choice' op all modules must have the same convention}}
-    %inst_in, %inst_out = firrtl.instance_choice inst sym @sym @Target alternatives @Platform {
-      @FPGA -> @PublicTarget
-    } (in in: !firrtl.vector<uint<8>, 2>, out out: !firrtl.vector<uint<8>, 2>)
-  }
-}

test/Dialect/FIRRTL/lower-signatures.mlir

@@ -132,14 +132,13 @@ firrtl.circuit "InstanceChoice" {
   firrtl.module @TargetWithDomain(
     in %D: !firrtl.domain<@ClockDomain()>,
     in %b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [%D]
-  ) attributes {convention = #firrtl<convention scalarized>} {
+  ) {
   }
 
-  firrtl.module @FPGATargetWithDomain(
-    in %D: !firrtl.domain<@ClockDomain()>,
-    in %b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [%D]
-  ) attributes {convention = #firrtl<convention scalarized>} {
-  }
+  firrtl.extmodule @FPGATargetWithDomain(
+    in D: !firrtl.domain<@ClockDomain()>,
+    in b: !firrtl.bundle<x: uint<1>, y: uint<2>> domains [D]
+  )
 
   firrtl.module @ASICTargetWithDomain(
     in %D: !firrtl.domain<@ClockDomain()>,

test/Dialect/FIRRTL/lower-types.mlir

@@ -1568,6 +1568,10 @@ firrtl.circuit "InstanceChoiceTest" {
     firrtl.matchingconnect %1, %0 : !firrtl.uint<8>
   }
 
+  firrtl.extmodule private @Ext(
+    in in: !firrtl.bundle<a: uint<8>, b: uint<8>>,
+    out out: !firrtl.bundle<x: uint<8>, y: uint<8>>
+  )
 
   // CHECK-LABEL: firrtl.module @InstanceChoiceTest
   firrtl.module @InstanceChoiceTest(
@@ -1576,10 +1580,10 @@ firrtl.circuit "InstanceChoiceTest" {
     out %out_x: !firrtl.uint<8>,
     out %out_y: !firrtl.uint<8>
   ) {
-    // CHECK: %inst_in_a, %inst_in_b, %inst_out_x, %inst_out_y = firrtl.instance_choice inst @TargetModule alternatives @Platform
+    // CHECK: %inst_in_a, %inst_in_b, %inst_out_x, %inst_out_y = firrtl.instance_choice inst @Ext alternatives @Platform
     // CHECK-SAME: @FPGA -> @TargetModule
     // CHECK-SAME: (in in_a: !firrtl.uint<8>, in in_b: !firrtl.uint<8>, out out_x: !firrtl.uint<8>, out out_y: !firrtl.uint<8>)
-    %inst_in, %inst_out = firrtl.instance_choice inst @TargetModule alternatives @Platform {
+    %inst_in, %inst_out = firrtl.instance_choice inst @Ext alternatives @Platform {
       @FPGA -> @TargetModule
     } (in in: !firrtl.bundle<a: uint<8>, b: uint<8>>, out out: !firrtl.bundle<x: uint<8>, y: uint<8>>)