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",