zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
498 stars 50 forks source link

package parameter wrong naming when imported #190

Closed ReinBentdal closed 2 years ago

ReinBentdal commented 2 years ago

When importing package parameters,

package MyPackage;
    localparam PARAM = 4;
endpackage

module Test;
    import MyPackage::PARAM;

    logic [PARAM] test;
endmodule

when compiled to Verilog we get,

module Test;
    localparam MyPackage_PARAM;

    logic [PARAM] test;
endmodule

This will give an error since MyPackage_PARAM and PARAM have different names. I would suggest changing the behavior to set the parameter name to PARAM instead of MyPackage_PARAM.

Love this tool!

zachjs commented 2 years ago

What version of sv2v are you using? This can be found using sv2v --version.

On all the recent versions, I get:

module Test;
    localparam MyPackage_PARAM = 4;
    wire [0:3] test;
endmodule

Note that MyPackage_PARAM - 1 is automatically simplified to 3 in the dimensions of test.

Perhaps there is an issue, but this reproduction example isn't quite right? If so, the issue likely isn't that MyPackage::PARAM becomes MyPackage_PARAM, but rather that the subsequent usage of PARAM is not appropriately being renamed.

ReinBentdal commented 2 years ago

I am using version 0.0.9. It looks like it works as you described in most cases, but sometimes it doesn't simplify. Her I have an example. This was generated with sv2v:

module PIXEL_SENSOR (
    ANALOG_RAMP,
    ERASE,
    EXPOSE,
    READ,
    DIGITAL_RAMP,
    DATA
);
    input ANALOG_RAMP;
    input ERASE;
    input EXPOSE;
    input READ;
    input [PIXEL_BITS - 1:0] DIGITAL_RAMP; // NOT SIMPLIFIED
    output wire [PIXEL_BITS - 1:0] DATA; // NOT SIMPLIFIED
    parameter integer width_index = 0;
    parameter integer height_index = 0;
    localparam PixelSensorConfig_PIXEL_BITS = 8;
    reg [7:0] local_data;
    Tristate Tristate[7:0](
        .A(local_data),
        .EN(READ),
        .Y(DATA)
    );
    wire cmp;
    always @(*)
        if (!cmp)
            local_data = DIGITAL_RAMP;
    PIXEL_SENSOR_ANALOG #(
        .width_index(width_index),
        .height_index(height_index)
    ) PixelSensorAnalog(
        .EXPOSE(EXPOSE),
        .RAMP(ANALOG_RAMP),
        .ERASE(ERASE),
        .CMP(cmp)
    );
endmodule

from this code

`include "../../pixel_sensor_config.sv"
`include "../../components/tristate.sv"
`include "../pixel_sensor/pixelSensorAnalog.sv"

module PIXEL_SENSOR
  (
   input ANALOG_RAMP,
   input ERASE,
   input EXPOSE,
   input READ,
   input [PIXEL_BITS-1:0] DIGITAL_RAMP,
   output [PIXEL_BITS-1:0] DATA
   );

   import PixelSensorConfig::PIXEL_BITS;
   import PixelSensorConfig::PIXEL_ARRAY_WIDTH;
   import PixelSensorConfig::PIXEL_ARRAY_HEIGHT;

   parameter integer width_index = 0;
   parameter integer height_index = 0;

   logic [PIXEL_BITS-1:0] local_data;

   Tristate Tristate[PIXEL_BITS-1:0](
      .A(local_data),
      .EN(READ),
      .Y(DATA)
   );

   logic cmp;

   // uses latch to store ADC value
   always_latch begin
      if (!cmp)
         local_data = DIGITAL_RAMP;
   end

   // PIXEL SENSOR and comparator digital model of analog circuit
   PIXEL_SENSOR_ANALOG #(
      .width_index(width_index),
      .height_index(height_index)
   ) PixelSensorAnalog(
      .EXPOSE(EXPOSE),
      .RAMP(ANALOG_RAMP),
      .ERASE(ERASE),
      .CMP(cmp)
   );

endmodule
ReinBentdal commented 2 years ago

To me it maybe looks like inputs and outputs are not simplified

zachjs commented 2 years ago

Thank you very much for providing that additional information!

In SystemVerilog, the usage of an identifier before it has been declared is illegal. This means that an identifier imported from a package cannot be used prior to the corresponding import. As such, the following simplified example is illegal and is correctly rejected by 3 of the 4 commercial simulators available on edaplayground.com.

package P;
    localparam X = 5;
endpackage
module top (
    output logic [X - 1:0] o
);
    import P::X; // Too late!
    initial $display("%b", o);
endmodule

However, SystemVerilog has a clever solution for this problem! Package import declarations can be placed in the module header before the parameters and ports, allowing them to reference the imported items. The following example is legal, and is accepted by all 4 of those commercial simulators, and also works as expected in sv2v.

package P;
    localparam X = 5;
endpackage
module top
    import P::X;
(
    output logic [X - 1:0] o
);
    initial $display("%b", o);
endmodule
ReinBentdal commented 2 years ago

Ahh thanks a lot for the detailed explanation! I will go a head and fix my code straight a head.

Appreciate the work done on this project. Have certainly helped me allot!

ReinBentdal commented 2 years ago

I can confirm this fixed my issue

zachjs commented 2 years ago

That's great to hear! I'm glad that worked for you.