[FIRRTL][Dedup] Rework hashing for perf and bug fixes. (#7420)

Primary change is to only generate and populate mappings for
sources of values, and not each value themselves.

Values are identified using these base numberings plus an appropriate
offset.

The main benefit of this is to greatly reduce the number of entries in the
`indices` map.
When handling operations with many block arguments (module-like's with many
ports) or with many results (instances of those module-like's) this greatly
reduces the pressure on the `indices` map.  For these designs, dedup now runs
dramatically faster and uses significantly less memory.

Also separates location of the value impl, such that if a Value's impl is storage
inline into an Operation or Block such that there is aliasing, the two
are given different numbers (and especially the numbering isn't changed).

On a synthetic design containing a module with 2^20 ports and 256 instances of
that module, this is the difference between completing in 20s and OOM'ing on my
machine after running for 30 minutes.

Functional changes:

Fixes #7415.
Fixes #7416.
Also fixes deduping if block arg types are different (but unused).

This is done by hashing block count, and each block's numbering between
as well as the types of its arguments before that block's operations.

Additionally fixes use of numberings (indices) before it was
populated where attribute processing for inner symbol ports
hashed using the block argument's numbering before it was populated.

Author: dtzSiFive

lib/Dialect/FIRRTL/Transforms/Dedup.cpp

@@ -78,8 +78,17 @@ struct ModuleInfo {
   mlir::ArrayAttr referredModuleNames;
 };
 
-struct SymbolTarget {
+/// Unique identifier for a value.  All value sources are numbered by apperance,
+/// and values are identified using this numbering (`index`) and an `offset`.
+/// For BlockArgument's, this is the argument number.
+/// For OpResult's, this is the result number.
+struct ValueId {
   uint64_t index;
+  uint64_t offset;
+};
+
+struct SymbolTarget {
+  ValueId index;
   uint64_t fieldID;
 };
 
@@ -161,14 +170,19 @@ struct StructuralHasher {
 
   void record(void *address) {
     auto size = indices.size();
+    assert(!indices.contains(address));
     indices[address] = size;
   }
 
-  void update(BlockArgument arg) { record(arg.getAsOpaquePointer()); }
+  /// Get the unique id for the specified value.
+  ValueId getId(Value val) {
+    if (auto arg = dyn_cast<BlockArgument>(val))
+      return {indices.at(arg.getOwner()), arg.getArgNumber()};
+    auto result = cast<OpResult>(val);
+    return {indices.at(result.getOwner()), result.getResultNumber()};
+  }
 
   void update(OpResult result) {
-    record(result.getAsOpaquePointer());
-
     // Like instance ops, don't use object ops' result types since they might be
     // replaced by dedup. Record the class names and lazily combine their hashes
     // using the same mechanism as instances and modules.
@@ -180,23 +194,23 @@ struct StructuralHasher {
     update(result.getType());
   }
 
-  void update(OpOperand &operand) {
-    // We hash the value's index as it apears in the block.
-    auto it = indices.find(operand.get().getAsOpaquePointer());
-    assert(it != indices.end() && "op should have been previously hashed");
-    update(it->second);
+  void update(ValueId index) {
+    update(index.index);
+    update(index.offset);
   }
 
+  void update(OpOperand &operand) { update(getId(operand.get())); }
+
   void update(Operation *op, hw::InnerSymAttr attr) {
     for (auto props : attr)
       innerSymTargets[props.getName()] =
-          SymbolTarget{indices[op], props.getFieldID()};
+          SymbolTarget{{indices.at(op), 0}, props.getFieldID()};
   }
 
   void update(Value value, hw::InnerSymAttr attr) {
     for (auto props : attr)
       innerSymTargets[props.getName()] =
-          SymbolTarget{indices[value.getAsOpaquePointer()], props.getFieldID()};
+          SymbolTarget{getId(value), props.getFieldID()};
   }
 
   void update(const SymbolTarget &target) {
@@ -281,15 +295,6 @@ struct StructuralHasher {
     }
   }
 
-  void update(Block &block) {
-    // Hash the block arguments.
-    for (auto arg : block.getArguments())
-      update(arg);
-    // Hash the operations in the block.
-    for (auto &op : block)
-      update(&op);
-  }
-
   void update(mlir::OperationName name) {
     // Operation names are interned.
     update(name.getAsOpaquePointer());
@@ -299,26 +304,44 @@ struct StructuralHasher {
   void update(Operation *op) {
     record(op);
     update(op->getName());
-    update(op, op->getAttrDictionary());
+
     // Hash the operands.
     for (auto &operand : op->getOpOperands())
       update(operand);
+
+    // Number the block pointers, for use numbering their arguments.
+    for (auto &region : op->getRegions())
+      for (auto &block : region.getBlocks())
+        record(&block);
+
+    // This happens after the numbering above, as it uses blockarg numbering
+    // for inner symbols.
+    update(op, op->getAttrDictionary());
+
     // Hash the regions. We need to make sure an empty region doesn't hash the
     // same as no region, so we include the number of regions.
     update(op->getNumRegions());
-    for (auto &region : op->getRegions())
-      for (auto &block : region.getBlocks())
-        update(block);
-    // Record any op results.
+    for (auto &region : op->getRegions()) {
+      update(region.getBlocks().size());
+      for (auto &block : region.getBlocks()) {
+        update(indices.at(&block));
+        for (auto argType : block.getArgumentTypes())
+          update(argType);
+        for (auto &op : block)
+          update(&op);
+      }
+    }
+
+    // Record any op results (types).
     for (auto result : op->getResults())
       update(result);
   }
 
-  // Every operation and value is assigned a unique id based on their order of
-  // appearance
+  // Every operation and block is assigned a unique id based on their order of
+  // appearance.  All values are uniquely identified using these.
   DenseMap<void *, unsigned> indices;
 
-  // Every value is assigned a unique id based on their order of appearance.
+  // Track inner symbol name -> target's unique identification.
   DenseMap<StringAttr, SymbolTarget> innerSymTargets;
 
   // This keeps track of module names in the order of the appearance.

test/Dialect/FIRRTL/dedup-errors.mlir

@@ -290,6 +290,95 @@ firrtl.circuit "MustDedup" attributes {annotations = [{
 
 // -----
 
+// Check same number of blocks but instructions across are same.
+// https://github.com/llvm/circt/issues/7415
+// expected-error@below {{module "Test1" not deduplicated with "Test0"}}
+firrtl.circuit "SameInstDiffBlock" attributes {annotations = [{
+      class = "firrtl.transforms.MustDeduplicateAnnotation",
+      modules = ["~SameInstDiffBlock|Test0", "~SameInstDiffBlock|Test1"]
+    }]} {
+  firrtl.module private @Test0(in %a : !firrtl.uint<1>) {
+    "test"()({
+      ^bb0:
+        // expected-note@below {{first block has more operations}}
+        "return"() : () -> ()
+    }, {
+      ^bb0:
+    }) : () -> ()
+  }
+  firrtl.module private @Test1(in %a : !firrtl.uint<1>) {
+    // expected-note@below {{second block here}}
+    "test"() ({
+      ^bb0:
+    }, {
+      ^bb0:
+        "return"() : () -> ()
+    }): () -> ()
+  }
+  firrtl.module @SameInstDiffBlock() {
+    firrtl.instance test0 @Test0(in a : !firrtl.uint<1>)
+    firrtl.instance test1 @Test1(in a : !firrtl.uint<1>)
+  }
+}
+
+// -----
+
+// Check differences in block arguments.
+// expected-error@below {{module "Test1" not deduplicated with "Test0"}}
+firrtl.circuit "BlockArgTypes" attributes {annotations = [{
+      class = "firrtl.transforms.MustDeduplicateAnnotation",
+      modules = ["~BlockArgTypes|Test0", "~BlockArgTypes|Test1"]
+    }]} {
+  firrtl.module private @Test0(in %a : !firrtl.uint<1>) {
+    // expected-note@below {{types don't match, first type is 'i32'}}
+    "test"()({
+      ^bb0(%arg0 : i32):
+        "return"() : () -> ()
+    }) : () -> ()
+  }
+  firrtl.module private @Test1(in %a : !firrtl.uint<1>) {
+    // expected-note@below {{second type is 'i64'}}
+    "test"() ({
+      ^bb0(%arg0 : i64):
+        "return"() : () -> ()
+    }): () -> ()
+  }
+  firrtl.module @BlockArgTypes() {
+    firrtl.instance test0 @Test0(in a : !firrtl.uint<1>)
+    firrtl.instance test1 @Test1(in a : !firrtl.uint<1>)
+  }
+}
+
+// -----
+
+// Check empty block not same as no block.
+// https://github.com/llvm/circt/issues/7416
+// expected-error@below {{module "B" not deduplicated with "A"}}
+firrtl.circuit "NoBlockEmptyBlock" attributes {annotations = [{
+      class = "firrtl.transforms.MustDeduplicateAnnotation",
+      modules = ["~NoBlockEmptyBlock|A", "~NoBlockEmptyBlock|B"]
+    }]} {
+  firrtl.module private @A(in %x: !firrtl.uint<1>) {
+    // expected-note @below {{operation regions have different number of blocks}}
+    firrtl.when %x : !firrtl.uint<1> {
+    }
+  }
+  firrtl.module private @B(in %x: !firrtl.uint<1>) {
+    // expected-note @below {{second operation here}}
+    firrtl.when %x : !firrtl.uint<1> {
+    } else {
+    }
+  }
+  firrtl.module @NoBlockEmptyBlock(in %x: !firrtl.uint<1>) {
+    %a_x = firrtl.instance a @A(in x: !firrtl.uint<1>)
+    firrtl.matchingconnect %a_x, %x : !firrtl.uint<1>
+    %b_x = firrtl.instance b @B(in x: !firrtl.uint<1>)
+    firrtl.matchingconnect %b_x, %x : !firrtl.uint<1>
+  }
+}
+
+// -----
+
 // expected-error@below {{module "Test1" not deduplicated with "Test0"}}
 firrtl.circuit "MustDedup" attributes {annotations = [{
       class = "firrtl.transforms.MustDeduplicateAnnotation",