From 20cb546d18a28ebf7a2e8dc87102a53a8da33bc3 Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Thu, 1 Aug 2024 07:02:53 -0500 Subject: [PATCH] [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. --- lib/Dialect/FIRRTL/Transforms/Dedup.cpp | 79 ++++++++++++++-------- test/Dialect/FIRRTL/dedup-errors.mlir | 89 +++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 28 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/Dedup.cpp b/lib/Dialect/FIRRTL/Transforms/Dedup.cpp index 560aa0e3d7a0..1fb2ad2fe9aa 100644 --- a/lib/Dialect/FIRRTL/Transforms/Dedup.cpp +++ b/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(val)) + return {indices.at(arg.getOwner()), arg.getArgNumber()}; + auto result = cast(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 ®ion : 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 ®ion : op->getRegions()) - for (auto &block : region.getBlocks()) - update(block); - // Record any op results. + for (auto ®ion : 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 indices; - // Every value is assigned a unique id based on their order of appearance. + // Track inner symbol name -> target's unique identification. DenseMap innerSymTargets; // This keeps track of module names in the order of the appearance. diff --git a/test/Dialect/FIRRTL/dedup-errors.mlir b/test/Dialect/FIRRTL/dedup-errors.mlir index f0a8639041d3..19355c0e4a38 100644 --- a/test/Dialect/FIRRTL/dedup-errors.mlir +++ b/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",