[ImportVerilog] Dedup moduleOp

[mingzheTerapines]

Dedup module op generating while importing Verilog

[mingzheTerapines]

@fabianschuiki This PR can only dedup same name module OP generating which is more lightweight. This is only an option. https://github.com/llvm/circt/pull/7190/files That PR can dedup all equipllance module OP which is more powerful. This is a pass. They are two different methods of deup. How can I add an option for circt-verilog?

[fabianschuiki]

For adding options to circt-verilog, take a look at circt-verilog.cpp: you can define options there and then pass them through to ImportVerilog via the import options struct.

However, deduplication during import should be non-optional. The Verilog source text is already fully deduplicated. We want to capture that in the Moore IR and have that be deduplicated as well. It's unfortunate that Slang uniquifies the module for every instance, but that's just a Slang limitation we have to reverse when we import Verilog. So I would not add an option at all, but make this the default behavior instead.

Ideally Slang would only monomorphize parameters and not duplicate modules. But if I remember correctly, Mike Popolski had some good reasons to do this in v3.0. It might have changed since.

[fabianschuiki]

I think you need more than just the name for deduplication: instances of the same module with different parameters will have the same name, but wildly different ports and bodies. I think you need a wrapper struct around the module and use that as a hash map key: that should hash the entire module if asked for a hash, and structurally check for equivalence if asked for equivalence.

[mingzheTerapines]

@fabianschuiki you are right, this is a bug example

module top;
    module NestedA(input wire a,
    input wire b);
    endmodule
    module NestedB(input wire a,
    input wire b);
    endmodule
    wire [3:0] a;
    wire [3:0] b;
    NestedA insA(.a(a));
    NestedA insB(.b(b));
    NestedB insC();
    NestedB insD(.a(a), .b(b));
endmodule

module NestedA;
  module NestedB;
    module NestedC;
    endmodule
  endmodule
endmodule

DenseMap<const slang::ast::InstanceBodySymbol , std::unique_ptr> modules; -> llvm::ScopedHashTable<const slang::ast::InstanceBodySymbol , std::unique_ptr> modules; may solve this problem Also you mentioned hash map key is more powerful to dedup. I will also have a think.

[mingzheTerapines]

@fabianschuiki @hailongSun2000 @cepheus69 Comparing name is dangerous, now I also compare parent's symbol.

if (instNode.body.name == module.first->name &&
          instNode.getParentScope() == module.getFirst()->getParentScope()) {
        duplicateInstanceBodySymbol = module.first;
        break;
      }

[hailongSun2000]

Hey, @mingzheTerapines. There is an example with the parameter:

module Top(input [15:0] c16,
           input [3:0] c4,
           output [15:0] o16,
           output [3:0] o4);

  A #(16) a1(c16, o16);
  A #(4) a2(c4, o4);
endmodule

module A #(parameter p = 32)
         (input [p-1:0] cin,
          output [p-1:0] cout);
   // ...
endmodule        

module {
  moore.module @Top(in %c16 : !moore.l16, in %c4 : !moore.l4, out o16 : !moore.l16, out o4 : !moore.l4) {
    %c16_0 = moore.net name "c16" wire : <l16>
    %c4_1 = moore.net name "c4" wire : <l4>
    %o16 = moore.net wire : <l16>
    %o4 = moore.net wire : <l4>
    %0 = moore.read %c16_0 : l16
    %a1.cout = moore.instance "a1" @A(cin: %0: !moore.l16) -> (cout: !moore.l16)
    moore.assign %o16, %a1.cout : l16
    %1 = moore.read %c4_1 : l4
    %a2.cout = moore.instance "a2" @A_0(cin: %1: !moore.l4) -> (cout: !moore.l4)
    moore.assign %o4, %a2.cout : l4
    moore.assign %c16_0, %c16 : l16
    moore.assign %c4_1, %c4 : l4
    %2 = moore.read %o16 : l16
    %3 = moore.read %o4 : l4
    moore.output %2, %3 : !moore.l16, !moore.l4
  }
  moore.module @A_0(in %cin : !moore.l4, out cout : !moore.l4) {
    %0 = moore.constant 4 : l32
    %p = moore.named_constant parameter %0 : l32
    %cin_0 = moore.net name "cin" wire : <l4>
    %cout = moore.net wire : <l4>
    moore.assign %cin_0, %cin : l4
    %1 = moore.read %cout : l4
    moore.output %1 : !moore.l4
  }
  moore.module @A(in %cin : !moore.l16, out cout : !moore.l16) {
    %0 = moore.constant 16 : l32
    %p = moore.named_constant parameter %0 : l32
    %cin_0 = moore.net name "cin" wire : <l16>
    %cout = moore.net wire : <l16>
    moore.assign %cin_0, %cin : l16
    %1 = moore.read %cout : l16
    moore.output %1 : !moore.l16
  }
}

If you just compare the port name and module name, it will cause some problems. The instance a1 and a2 will have different results with different parameters. Obviously, the moore.output will produce a result with different width. And sometimes, the parameter will control the behavior of the module.

[mingzheTerapines]

@fabianschuiki I have supported TypeParameterSymbol dedup but construct is not ready yet. So I was wondering converting Typeparameter to hw.paramvalue, but found no mooreOPs support mlir::typeattribute declared in an Op. Maybe we need add NamedTypeConstant Op or something and typeType for Moore dialect. Also Thanks for @hailongSun2000's guiding, This PR seems good now.