Verilog import fails in macro substitution

[yannickl96]

I have a use case where I have several Verilog modules which implement similar behavior, but their I/O bit widths differ and they are not parametrizable to the bit width. Apart from the interface, the import "BVI"-code does not depend on the bit widths.
In order to save copy-paste code, I wanted to import all relevant Verilog modules using macro functions in BSV. However, the compiler fails due to conflicting interface declarations.
I built a minimum reproducing example based on the FIFOF_-imports. Here is the main part of it:

I have a common generic interface:

interface FIFOF_ #(type a) ;
        method Action enq(a x1) ;
        method Action deq() ;
        method a first() ;
        method Action clear() ;
        method Bool notFull() ;
        method Bool i_notFull() ;
        method Bool notEmpty() ;
        method Bool i_notEmpty() ;
endinterface: FIFOF_

and I want to bind a module with types Bit#(8) and Bit#(16) for type a. If I just hardcode the imports, everything works fine (FIFO1 is just the FIFO1 Verilog module with width set to 8 and FIFO1_16 sets width to 16):

import "BVI" FIFO1 =
        module vMk1FIFO1(FIFOF_#(Bit#(8)));
          default_clock clk(CLK, (*unused*)CLK_GATE);
          method enq((* reg *)D_IN) enable(ENQ);
          method deq() enable(DEQ);
          method (* reg *)D_OUT first;
          method (* reg *)FULL_N   notFull;
          method (* reg *)FULL_N i_notFull;
          method (* reg *)EMPTY_N   notEmpty;
          method (* reg *)EMPTY_N i_notEmpty;
          method clear() enable(CLR);
          schedule deq CF (enq, i_notEmpty, i_notFull) ;
          schedule enq CF (deq, first, i_notEmpty, i_notFull) ;
          schedule (first, notEmpty, notFull) CF
                     (first, i_notEmpty, i_notFull, notEmpty, notFull) ;
          schedule (i_notEmpty, i_notFull) CF
                     (clear, first, i_notEmpty, i_notFull, notEmpty, notFull) ;
          schedule (clear, deq, enq) SBR clear ;
          schedule first SB (clear, deq) ;
          schedule (notEmpty, notFull) SB (clear, deq, enq) ;
          schedule deq C deq;
          schedule enq C enq;
        endmodule: vMk1FIFO1

    import "BVI" FIFO1_16 =
        module vMk1FIFO1_16(FIFOF_#(Bit#(16)));
          default_clock clk(CLK, (*unused*)CLK_GATE);
          method enq((* reg *)D_IN) enable(ENQ);
          method deq() enable(DEQ);
          method (* reg *)D_OUT first;
          method (* reg *)FULL_N   notFull;
          method (* reg *)FULL_N i_notFull;
          method (* reg *)EMPTY_N   notEmpty;
          method (* reg *)EMPTY_N i_notEmpty;
          method clear() enable(CLR);
          schedule deq CF (enq, i_notEmpty, i_notFull) ;
          schedule enq CF (deq, first, i_notEmpty, i_notFull) ;
          schedule (first, notEmpty, notFull) CF
                     (first, i_notEmpty, i_notFull, notEmpty, notFull) ;
          schedule (i_notEmpty, i_notFull) CF
                     (clear, first, i_notEmpty, i_notFull, notEmpty, notFull) ;
          schedule (clear, deq, enq) SBR clear ;
          schedule first SB (clear, deq) ;
          schedule (notEmpty, notFull) SB (clear, deq, enq) ;
          schedule deq C deq;
          schedule enq C enq;
        endmodule: vMk1FIFO1_16

If I refactor common code into a macro I get:

`define BIND_FIFOF(vmodule, bitwidth) \
        import "BVI" vmodule = \
        module vMk1``vmodule``(FIFOF_#(Bit#(bitwidth))); \
          default_clock clk(CLK, (*unused*)CLK_GATE); \
          method enq((* reg *)D_IN) enable(ENQ); \
          method deq() enable(DEQ); \
          method (* reg *)D_OUT first; \
          method (* reg *)FULL_N   notFull; \
          method (* reg *)FULL_N i_notFull; \
          method (* reg *)EMPTY_N   notEmpty; \
          method (* reg *)EMPTY_N i_notEmpty; \
          method clear() enable(CLR); \
          schedule deq CF (enq, i_notEmpty, i_notFull) ; \
          schedule enq CF (deq, first, i_notEmpty, i_notFull) ; \
          schedule (first, notEmpty, notFull) CF \
                     (first, i_notEmpty, i_notFull, notEmpty, notFull) ; \
          schedule (i_notEmpty, i_notFull) CF \
                     (clear, first, i_notEmpty, i_notFull, notEmpty, notFull) ; \
          schedule (clear, deq, enq) SBR clear ; \
          schedule first SB (clear, deq) ; \
          schedule (notEmpty, notFull) SB (clear, deq, enq) ; \
          schedule deq C deq; \
          schedule enq C enq; \
        endmodule

The imports then should be obtainable by invoking:

`BIND_FIFOF(FIFO1, 8)
`BIND_FIFOF(FIFO1_16, 16)

I wrote a test module instantiating the imported FIFOs. However, it fails with the following error:

$ bsc -u -verilog -g mkMWRE MWRE.bsv
checking package dependencies
compiling MWRE.bsv
Error: "MWRE.bsv", line 67, column 16: (T0011)
  Declaration of `_MWRE.FIFOF_67' conflicts with previous declaration at
  "MWRE.bsv", line 67, column 16

I checked the output of the preprocessor (by passing the -E flag to bsc) for the macro invocations and from my understanding, the line directive sets the file and line information for the diagnostics messages to the specified values:

`line(MWRE.bsv,89,1,0)    
     `line(MWRE.bsv,65,42,1) 
        import "BVI" FIFO1 = 
        module vMk1FIFO1(FIFOF_#(Bit#( 8))); 
          default_clock clk(CLK, (*unused*)CLK_GATE); 
          method enq((* reg *)D_IN) enable(ENQ); 
          method deq() enable(DEQ); 
          method (* reg *)D_OUT first; 
          method (* reg *)FULL_N   notFull; 
          method (* reg *)FULL_N i_notFull; 
          method (* reg *)EMPTY_N   notEmpty; 
          method (* reg *)EMPTY_N i_notEmpty; 
          method clear() enable(CLR); 
          schedule deq CF (enq, i_notEmpty, i_notFull) ; 
          schedule enq CF (deq, first, i_notEmpty, i_notFull) ; 
          schedule (first, notEmpty, notFull) CF 
                     (first, i_notEmpty, i_notFull, notEmpty, notFull) ; 
          schedule (i_notEmpty, i_notFull) CF 
                     (clear, first, i_notEmpty, i_notFull, notEmpty, notFull) ; 
          schedule (clear, deq, enq) SBR clear ; 
          schedule first SB (clear, deq) ; 
          schedule (notEmpty, notFull) SB (clear, deq, enq) ; 
          schedule deq C deq; 
          schedule enq C enq; 
        endmodule
 `line(MWRE.bsv,90,26,2)

     `line(MWRE.bsv,65,42,1) 
        import "BVI" FIFO1_16 = 
        module vMk1FIFO1_16(FIFOF_#(Bit#( 16))); 
          default_clock clk(CLK, (*unused*)CLK_GATE); 
          method enq((* reg *)D_IN) enable(ENQ); 
          method deq() enable(DEQ); 
          method (* reg *)D_OUT first; 
          method (* reg *)FULL_N   notFull; 
          method (* reg *)FULL_N i_notFull; 
          method (* reg *)EMPTY_N   notEmpty; 
          method (* reg *)EMPTY_N i_notEmpty; 
          method clear() enable(CLR); 
          schedule deq CF (enq, i_notEmpty, i_notFull) ; 
          schedule enq CF (deq, first, i_notEmpty, i_notFull) ; 
          schedule (first, notEmpty, notFull) CF 
                     (first, i_notEmpty, i_notFull, notEmpty, notFull) ; 
          schedule (i_notEmpty, i_notFull) CF 
                     (clear, first, i_notEmpty, i_notFull, notEmpty, notFull) ; 
          schedule (clear, deq, enq) SBR clear ; 
          schedule first SB (clear, deq) ; 
          schedule (notEmpty, notFull) SB (clear, deq, enq) ; 
          schedule deq C deq; 
          schedule enq C enq; 
        endmodule
 `line(MWRE.bsv,92,30,2)

This leads me to think, that during the second BVI import, the compiler declares an interface FIFOF_67 according to the set source line number which then conflicts with the interface declared in the previous BIND_FIFOmacro invocation.
Invoking the macro only once works, but then I wouldn't need a macro.
When I use the preprocessed code as input source file into the compiler, everything works, if I change the line number in the second line(MWRE.bsv,65,42,1) invocation to something arbitrarily different.

Is there a way to get around this issue? My feeling is that the intention for the reset line numbers is to provide diagnostics for errors inside macros which makes complete sense from my point of view. Is it really necessary to re-declare interfaces for each imported Verilog module?

I attached my entire code (failing code, preprocessor output, verilog modules) to this issue. You can trigger the error with

$ bsc -u -verilog -g mkMWRE MWRE.bsv

Best,
Yannick

bvi_mwre.zip

[quark17]

Oh wow. Yeah, this is a combination of a number of quirks, that come together to cause trouble. Sorry for that.

It may be best to just write a script external to BSV which generates the BSV imports for all the modules. You could also see if the BH syntax allows expressing it better than BSV. But if you do want to try expressing this entirely in BSV, one workaround that I found is this:

`define BIND_FIFOF(uniqueid, vmodule, bitwidth) \
`line(BIND_FIFOF,uniqueid,1,1) \
        import "BVI" vmodule = \
...

`BIND_FIFOF(1, FIFO1, 8)
`BIND_FIFOF(2, FIFO1_16, 16)

I tried to make the unique ID implicit, by making it a macro definition that gets updated each time you call BIND_FIFOF, but ran into difficulties putting a define inside a define (and I'm not sure if that's just a limitation of the SVpp syntax or of BSC's implementation). But being able to control the position with a uniqueid argument does allow you to give distinguishing position information to each macro call, which could be helpful if you did get an error message pointing to the inside of the macro.

BSV's import-BVI syntax is really a wrapper around the very limited import that BSC supports in its internal representation. If you import a Verilog module into BSV with name mkMod and interface Ifc, what is generated in BSC's internal representation is: (1) a mkMod_raw which imports the Verilog into a raw interface Ifc_raw and (2) a wrapper module mkMod which instantiates mkMod_raw and converts its interface into Ifc. I used the names _raw for illustration, but it looks like BSC is using the position information to create the unique names. And that breaks when using macros that cause multiple imports to have the same position.

In BH, the import of raw Verilog is an expression. BH users currently have to manually write the wrapper around an import. But this does provide extra flexibility, because the import is an expression, which can appear inside code, like functions. So you might be able to do this in BH using an ordinary function in place of the macro call. There are some features that BSV provides that BH's import does not (yet), around multiple clocks I think, but it looks like you're not doing anything advanced, so BH syntax might be sufficient. (Note that the wrapper does also include some additional internal code for recording the types of ports and user names, etc, which would need to be manually replicated if written in BH.)

The BH import syntax even lets you construct the string for the Verilog module name, which BSV doesn't. So for example, this would be how to import multiple RegUN modules:

interface VReg n =
    write :: Bit n -> PrimAction
    read :: Bit n

mkVImport :: Module (VReg n)
mkVImport =
  let modname = "RegUN_" +++ integerToString (valueOf n)
  in
    module verilog modname "CLK" {
        read = "Q_OUT"{reg};
        write = "D_IN"{reg} "EN";
    } [ read <> read,
        read < write,
        write << write ]

mkImport :: (IsModule m c, Bits a sa) => m (Reg a)
mkImport = liftModule $
    module
      _r :: VReg sa
      {-# hide #-}
      _r <- mkVImport
      let name = Valid (primGetModuleName _r)
      let t = typeOf (_ :: a)
      primSavePortType name "D_IN" t
      primSavePortType name "Q_OUT" t
      interface
        _read = unpack _r.read
        _write x = fromPrimAction (_r.write (pack x))


mkRegUN8 :: Module (Reg (Bit 8))
mkRegUN8 = mkImport

mkRegUN16 :: Module (Reg (Bit 16))
mkRegUN16 = mkImport

In your case, the imported interface would seem to already be in raw form, so maybe BSC doesn't need to create Ifc_raw. I guess we could enhance BSC to detect this situation and not create a raw interface when not needed. But actually, I think that Action is not in raw form, so there would still need to be a wrapper that converts between PrimAction and Action. It may be possible that BSC's internal representation could be extended to include all the wrapper stuff inside the import construct, so that new definitions aren't needed. But I'm not sure if that's really possible -- at some point the construct will need to be expanded into the wrapper code. But maybe if the expansion is done after type-checking, then it can avoid having to create new names.

There is already an open issue #386 asking to enhance the preprocessor to keep a stack of the multiple positions that result from macro calls. If that was supported, then the Ifc_raw name could possibly be made more unique by using the multiple positions.

There may also be other ways to create unique names. But I think the line number was used because it helps the user connect the identifer back to their source code, if they saw it in a message from the compiler.

[yannickl96]

Thank you for the elaborate reply! For now I will stick with the macro workaround. Once I get more into BH I may switch to the BH variant. I really like that you can construct the Verilog module name as string and pass it to the import.