Name preserving translation

[cristian-mattarei]

The current version preserves only input and output ports of the main module. Not preserving the names of internal signals gets very hard to analyze possible issues, and debug unexpected behaviors.

Can this issue be resolved in the VerilogToCoreIR pass or does it require modifications in Yosys?

[dillonhuff]

It should be fixable by making changes to VerilogToCoreIR (and possibly to coreir). That will take work but I don't see any reason why we would need to change yosys.

The only significant problem is that verilog gives names to wires but coreir does not, so the wire names will have to be some sort of metadata.

[cristian-mattarei]

As a reference, one example is the oh_fifo_generic.v (https://github.com/parallella/oh/blob/master/src/common/hdl/oh_fifo_generic.v) model.

[dillonhuff]

@cristian-mattarei I have added wire metadata to the generated coreir.

Every module in the design now has an instance wire map in its metadata. This map has an entry for every instance in the module.

Each of these instance entries contains a map from ports on the instance to the list of wires connected to that port.

Let me know if you have any questions.

[dillonhuff]

So for example this verilog file:

module add_fan_out(input [1:0] in0,
                   input [1:0]  in1,
                   output [1:0] out0,
                   output [1:0] out1,
                   input        clk);

   reg [1:0]                    res0;
   reg [1:0]                    res1;

   always @(posedge clk)
     begin
        res0 <= in0 + in1;
        res1 <= in0 - in1;
     end

   assign out0 = res0;
   assign out1 = res1;

endmodule // add_fan_out

Now has a metadata section in its generated coreir that maps each bit of each instance port to the wire that drives or receives it.

{"top":"global.add_fan_out",
"namespaces":{
  "global":{
    "modules":{
      "add_fan_out":{
        "type":["Record",[
          ["clk","BitIn"],
          ["in0",["Array",2,"BitIn"]],
          ["in1",["Array",2,"BitIn"]],
          ["out0",["Array",2,"Bit"]],
          ["out1",["Array",2,"Bit"]]
        ]],
        "instances":{
          "__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2":{
            "genref":"rtlil.add",
            "genargs":{"A_SIGNED":["Bool",false], "A_WIDTH":["Int",2], "B_SIGNED":["Bool",false], "B_WIDTH":["Int",2], "Y_WIDTH":["Int",2]}
          },
          "__DOLLAR__procdff__DOLLAR__4":{
            "genref":"rtlil.dff",
            "genargs":{"CLK_POLARITY":["Bool",true], "WIDTH":["Int",2]}
          },
          "__DOLLAR__procdff__DOLLAR__5":{
            "genref":"rtlil.dff",
            "genargs":{"CLK_POLARITY":["Bool",true], "WIDTH":["Int",2]}
          },
          "__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3":{
            "genref":"rtlil.sub",
            "genargs":{"A_SIGNED":["Bool",false], "A_WIDTH":["Int",2], "B_SIGNED":["Bool",false], "B_WIDTH":["Int",2], "Y_WIDTH":["Int",2]}
          }
        },
        "connections":[
          ["self.in0.0","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.A.0"],
          ["self.in0.1","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.A.1"],
          ["self.in1.0","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.B.0"],
          ["self.in1.1","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.B.1"],
          ["__DOLLAR__procdff__DOLLAR__4.D.0","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.Y.0"],
          ["__DOLLAR__procdff__DOLLAR__4.D.1","__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2.Y.1"],
          ["self.clk","__DOLLAR__procdff__DOLLAR__4.CLK"],
          ["self.out0.0","__DOLLAR__procdff__DOLLAR__4.Q.0"],
          ["self.out0.1","__DOLLAR__procdff__DOLLAR__4.Q.1"],
          ["self.clk","__DOLLAR__procdff__DOLLAR__5.CLK"],
          ["__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.Y.0","__DOLLAR__procdff__DOLLAR__5.D.0"],
          ["__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.Y.1","__DOLLAR__procdff__DOLLAR__5.D.1"],
          ["self.out1.0","__DOLLAR__procdff__DOLLAR__5.Q.0"],
          ["self.out1.1","__DOLLAR__procdff__DOLLAR__5.Q.1"],
          ["self.in0.0","__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.A.0"],
          ["self.in0.1","__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.A.1"],
          ["self.in1.0","__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.B.0"],
          ["self.in1.1","__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3.B.1"]
        ],
        "metadata":{"__DOLLAR__add__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__12__DOLLAR__2":{"A":["in0","in0"],"B":["in1","in1"],"Y":["$0\\res0[1:0]","$0\\res0[1:0]"]},"__DOLLAR__procdff__DOLLAR__4":{"CLK":["clk"],"D":["$0\\res0[1:0]","$0\\res0[1:0]"],"Q":["res0","res0"]},"__DOLLAR__procdff__DOLLAR__5":{"CLK":["clk"],"D":["$0\\res1[1:0]","$0\\res1[1:0]"],"Q":["res1","res1"]},"__DOLLAR__sub__DOLLAR____DOT____FORWARD_SLASH__test__FORWARD_SLASH__samples__FORWARD_SLASH__add_fan_out__FORWARD_SLASH__add_fan_out__DOT__v__COLON__13__DOLLAR__3":{"A":["in0","in0"],"B":["in1","in1"],"Y":["$0\\res1[1:0]","$0\\res1[1:0]"]}}
      }
    }
  }
}
}

[rdaly525]

I have been working on making the coreir->verilog translation much more readable and one main thing I have been doing is representing wires/nets as a "coreir.wire". I am thinking a good path forward for the verilog2Coreir flow is to explicitly instantiate a "coreir.wire" for every verilog wire. This will preserve names without needing any metadata.

[cristian-mattarei]

Thanks for doing that. How about for instance the "DOLLARaddDOLLAR__DOT__FORWARD_SLASHtestFORWARD_SLASHsamplesFORWARD_SLASHadd_fan_outFORWARD_SLASHadd_fan_outDOTvCOLON12DOLLAR2" instance? Is it possible to simplify/preserve these names?

[dillonhuff]

Those long names happen because yosys creates names that coreir will not accept because of special characters like / and $. We can create shorter abbreviations but we can't completely remove them.

If you'd like another scheme for mangling names to reduce size could you file another issue so that we can close this one?

[cristian-mattarei]

We should operate at the Yosys level in case, because even $add$./test/samples/add_fan_out/add_fan_out.v:12$2 looks like an unnecessary long name.

[dillonhuff]

Could you be more precise about what you mean by "operate at the yosys level in case"?

[cristian-mattarei]

If would prefer a naming scheme that simply represents the signals/names in the original model, and that keeps the naming as simple as possible such as add_01, add_02, add_03, ... in case there are multiple add modules that have been added during the translation. If this is not possible to be fixed in the VerilogToCoreIR, then we might have to change Yosys.

[cristian-mattarei]

In other words, if the instance comes from the original verilog, then preserve the name, while if it has been added internally during the translation, just use a naming scheme such as ex_add_01, ex_add_02, ex_add_03... that stands for EXternalADDer*

[dillonhuff]

I think I understand you. It seems like there are two different issues that need to be treated separately.

The first issue is that you would like for the names of objects that have been auto-generated by yosys during synthesis of RTL to have smaller, simpler names than they do now.

The reason generated names are so long is because they contain information about the position in the original source code that they were produced from (file name and line number). I want that information to be there, but if you don't like it you can simplify the auto generated names after conversion to coreir.

In yosys auto generated names start with a dollar sign. If you want to simplify auto-generated names you can write a coreir pass that finds all instances with auto generated names and re-names them. I'm sure @rdaly525 and I can help you with figuring out how to write an instance re-naming pass.

The second issue is that you would like the names that are present in the original verilog to be present in the generated coreir. There are several different things in the yosys RTL representation that have names:

1. Instantiated modules from the original verilog, eg:

adder this_is_a_name_for_an_adder_instance(.in0(a), .in1(b), .out(c));

2. Wires and registered wires from the original verilog e.g:

   wire [31:0] my_wire;
   reg [31:0] my_reg;

3. Other RTL cells created by yosys during synthesis. For example when the line:

   assign my_wire = my_reg + my_reg;

   is synthesized yosys will create an adder cell for the + operator. This cell will have an auto generated name.

Our translation already preserves names of type (1.). Names of type (2.) can be accessed through the metadata that I added yesterday, and could be accessed more easily if Ross's plan to add wire instances for yosys wires is implemented. Names of type (3.) end up as instance names in coreir. As I mentioned above they can be simplified after coreir generation if you don't like them.

I don't think there is any need to refactor or change yosys.

[dillonhuff]

@cristian-mattarei I have added a basic pass to rename modules that are auto generated and give them simpler names of the form instance_name_UNIQUE_NUMBER. This does not always lead to short names because parametric verilog modules have long automatically generated instance names, but it should be an improvement if you like short names.

It is in the pull request here and can be used like other coreir passes. Run deletedeadinstances after running this pass.