Implement a canonical form for FlipType's, ensuring they are always

propagated to the outer level of a type when possible.

include/spt/Dialect/FIRRTL/IR/Types.h

@@ -208,7 +208,7 @@ public:
   // Each element of a bundle, which is a name and type.
   using BundleElement = std::pair<Identifier, FIRRTLType>;
 
-  static BundleType get(ArrayRef<BundleElement> elements, MLIRContext *context);
+  static FIRRTLType get(ArrayRef<BundleElement> elements, MLIRContext *context);
 
   ArrayRef<BundleElement> getElements();
 
@@ -232,7 +232,7 @@ class FVectorType : public FIRRTLType::TypeBase<FVectorType, FIRRTLType,
 public:
   using Base::Base;
 
-  static FVectorType get(FIRRTLType elementType, unsigned numElements);
+  static FIRRTLType get(FIRRTLType elementType, unsigned numElements);
 
   FIRRTLType getElementType();
   unsigned getNumElements();

lib/Dialect/FIRRTL/Types.cpp

@@ -367,10 +367,56 @@ struct FlipTypeStorage : mlir::TypeStorage {
 } // namespace firrtl
 } // namespace spt
 
+/// Return a bundle type with the specified elements all flipped.  This assumes
+/// the elements list is non-empty.
+static FIRRTLType
+getFlippedBundleType(ArrayRef<BundleType::BundleElement> elements) {
+  assert(!elements.empty());
+  SmallVector<BundleType::BundleElement, 16> flippedelements;
+  flippedelements.reserve(elements.size());
+  for (auto &elt : elements)
+    flippedelements.push_back({elt.first, FlipType::get(elt.second)});
+  return BundleType::get(flippedelements, elements[0].second.getContext());
+}
+
 FIRRTLType FlipType::get(FIRRTLType element) {
-  // flip(flip(x)) -> x
-  if (auto subFlip = element.dyn_cast<FlipType>())
-    return subFlip.getElementType();
+  // We maintain a canonical form for flip types, where we prefer to have the
+  // flip as far outward from an otherwise passive type as possible.  If a flip
+  // is being used with an aggregate type that contains non-passive elements,
+  // then it is forced into the elements to get the canonical form.
+  switch (element.getKind()) {
+  case FIRRTLType::Clock:
+  case FIRRTLType::Reset:
+  case FIRRTLType::AsyncReset:
+  case FIRRTLType::SInt:
+  case FIRRTLType::UInt:
+  case FIRRTLType::Analog:
+    break;
+
+    // Derived Types
+  case FIRRTLType::Flip:
+    // flip(flip(x)) -> x
+    return element.cast<FlipType>().getElementType();
+
+  case FIRRTLType::Bundle: {
+    // If the bundle is passive, then we're done because the flip will be at the
+    // outer level. Otherwise, it contains flip types recursively within itself
+    // that we should canonicalize.
+    auto bundle = element.cast<BundleType>();
+    if (bundle.isPassiveType())
+      break;
+    return getFlippedBundleType(bundle.getElements());
+  }
+  case FIRRTLType::Vector: {
+    // If the bundle is passive, then we're done because the flip will be at the
+    // outer level. Otherwise, it contains flip types recursively within itself
+    // that we should canonicalize.
+    auto vec = element.cast<FVectorType>();
+    if (vec.isPassiveType())
+      break;
+    return FVectorType::get(get(vec.getElementType()), vec.getNumElements());
+  }
+  }
 
   // TODO: This should maintain a canonical form, digging any flips out of
   // sub-types.
@@ -420,8 +466,17 @@ struct BundleTypeStorage : mlir::TypeStorage {
 } // namespace firrtl
 } // namespace spt
 
-BundleType BundleType::get(ArrayRef<BundleElement> elements,
+FIRRTLType BundleType::get(ArrayRef<BundleElement> elements,
                            MLIRContext *context) {
+  // If all of the elements are flip types, then we canonicalize the flips to
+  // the outer level.
+  if (!elements.empty() &&
+      llvm::all_of(elements, [&](const BundleElement &elt) -> bool {
+        return elt.second.isa<FlipType>();
+      })) {
+    return FlipType::get(getFlippedBundleType(elements));
+  }
+
   return Base::get(context, Bundle, elements);
 }
 
@@ -486,7 +541,11 @@ struct VectorTypeStorage : mlir::TypeStorage {
 } // namespace firrtl
 } // namespace spt
 
-FVectorType FVectorType::get(FIRRTLType elementType, unsigned numElements) {
+FIRRTLType FVectorType::get(FIRRTLType elementType, unsigned numElements) {
+  // If elementType is a flip, then we canonicalize it outwards.
+  if (auto flip = elementType.dyn_cast<FlipType>())
+    return FlipType::get(FVectorType::get(flip.getElementType(), numElements));
+
   return Base::get(elementType.getContext(), Vector,
                    std::make_pair(elementType, numElements));
 }

test/fir/basic.fir

@@ -185,9 +185,9 @@ circuit basic :     ; CHECK: firrtl.circuit "basic" {
     reg _T_2622 : UInt<4>, clock with :
       reset => (UInt<1>("h0"), _T_2622)
 
-    ; CHECK: [[xyz:%.+]] = firrtl.instance @circuit {name = "xyz"} : !firrtl.bundle<in: flip<uint<80>>>
+    ; CHECK: [[xyz:%.+]] = firrtl.instance @circuit {name = "xyz"} : !firrtl.flip<bundle<in: uint<80>>>
     inst xyz of circuit
-    ; CHECK: [[xyz_in:%.+]] = firrtl.subfield [[xyz]]("in") : (!firrtl.bundle<in: flip<uint<80>>>) -> !firrtl.flip<uint<80>>
+    ; CHECK: [[xyz_in:%.+]] = firrtl.subfield [[xyz]]("in") : (!firrtl.flip<bundle<in: uint<80>>>) -> !firrtl.flip<uint<80>>
     ; CHECK: firrtl.connect [[xyz_in]], %i8 : !firrtl.flip<uint<80>>, !firrtl.uint<8>
     xyz.in <= i8
 
@@ -229,6 +229,16 @@ circuit basic :     ; CHECK: firrtl.circuit "basic" {
     wire _T_6 : { flip b : { bits : { source : UInt<7> } } }
     node _T_8 = bits(_T_6.b.bits.source, 5, 0)
 
+    ; CHECK: %flip1 = firrtl.wire {{.*}} !firrtl.bundle<x: bundle<a: uint>>
+    wire flip1 : { flip x : { flip a : UInt } }
+    ; CHECK: %flip2 = firrtl.wire {{.*}} !firrtl.bundle<x: bundle<a: uint, b: flip<analog>>>
+    wire flip2 : { flip x : { flip a : UInt, b: Analog } }
+    ; CHECK: %flip3 = firrtl.wire {{.*}} !firrtl.bundle<x: bundle<a: uint, b: analog>>
+    wire flip3 : { flip x : { flip a : UInt, flip b: Analog } }
+    ; CHECK: %flip4 = firrtl.wire {{.*}} !firrtl.bundle<x: vector<bundle<a: uint>, 4>>
+    wire flip4 : { flip x : { flip a : UInt }[4] }
+
+
   ; CHECK-LABEL: firrtl.module @expr_stmt_ambiguity(
   module expr_stmt_ambiguity :
     ; CHECK: %reg = firrtl.wire {name = "reg"} : !firrtl.uint