zachjs / sv2v

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

Name of modules that contain parameterized types #218

Closed kauser-rl closed 12 months ago

kauser-rl commented 1 year ago

I have a situation where I override the type of some signals via a parameter. The conversion to Verilog seems to be okay although for the default value of the type. I have two instances, one of which is non-default. There isn't a converted version of it. Any suggestions on how to work around it? I have pasted a test case below. There are three files, package, module with type and the wrapper that instances it.

mod_pkg.sv

`ifndef MOD_PKG
 `define MOD_PKG

package mod_pkg;

   typedef enum logic[1:0] {
     TYPEA_0  = 2'b00,
     TYPEA_1  = 2'b01,
     TYPEA_2  = 2'b10,
     TYPEA_3  = 2'b11
   } typea_t;

   typedef enum logic[2:0] {
     TYPEB_0  = 3'b000,
     TYPEB_1  = 3'b001,
     TYPEB_2  = 3'b010,
     TYPEB_3  = 3'b011,
     TYPEB_4  = 3'b100,
     TYPEB_5  = 3'b101,
     TYPEB_6  = 3'b110,
     TYPEB_7  = 3'b111
   } typeb_t;
endpackage // mod_pkg
`endif

mod_with_type.sv

`include "mod_pkg.sv"

module mod_with_type
  // Package imports
  import mod_pkg::*;
   #(parameter type  PR_TYPE = typea_t)
   (
    input wire PR_TYPE    type_i,
    output     logic      sel_o
   );

   parameter TYPE_W = $bits(typea_t);

   logic [TYPE_W-1:0] mask;

   generate if(TYPE_W==2) begin : g_typea
      assign mask = 2'b10;
   end else begin : g_typeb
      assign mask = 3'b101;
   end endgenerate

   assign sel_o = |(type_i & mask);
endmodule

top.sv

`include "mod_pkg.sv"
module top
  // Package imports
  import mod_pkg::*;
   ();

   typea_t typea;
   typeb_t typeb;
   logic sel_a;
   logic sel_b;

   mod_with_type #(.PR_TYPE (typea_t)) u_typea (.type_i (typea), .sel_o (sel_a));
   mod_with_type #(.PR_TYPE (typeb_t)) u_typeb (.type_i (typeb), .sel_o (sel_b));
endmodule // top
kauser-rl commented 1 year ago

The converted top.v is pasted below. You can see two instances, but none of those modules exist or are generated by sv2v.

module top;
    wire [1:0] typea;
    wire [2:0] typeb;
    wire sel_a;
    wire sel_b;
    mod_with_type_3D46F u_typea(
        .type_i(typea),
        .sel_o(sel_a)
    );
    mod_with_type_17C7C u_typeb(
        .type_i(typeb),
        .sel_o(sel_b)
    );
endmodule
zachjs commented 1 year ago

sv2v typically needs to be presented with all source files in a single invocation to work correctly. In this specific example, top.sv and mod_with_type.sv must be converted "simultaneously." I recognize this requirement is not obvious. I'd like to improve the documentation and perhaps issue a warning in such circumstances.

A couple options:

A) sv2v top.sv mod_with_type.sv will print the desired output to stdout. You can also use -w output.v to write to a file. B) sv2v -w adj top.sv mod_with_type.sv will produce two files, top.v and mod_with_type.v, with the desired output.

kauser-rl commented 1 year ago

I haven't tested it, but providing multiple dependent files might be an issue. See the typical way to provide a list of files for compilation is via a .vc file which contains search paths for all modules. For sv2v, we have to create a list of all the files to convert one at a time. This is inconvenient, but it is still manageable. If we have to pull out all dependent modules, either this would need to be manually maintained or we would need our own script to do it. How about using the search path to look for the dependent modules, just like you would for the include files?

zachjs commented 1 year ago

It sounds like you’re interested in a flag akin to -y in other tools, automatically loading an unrecognized some_module from a source file like some_module.sv found in the library search paths. Is that correct? If so, I will look into implementing this feature.

Regardless of the above, sv2v expects that it will convert an entire design in a single invocation. Currently, this means specifying every source file as an input, e.g., sv2v rtl/**/*.sv as seen in some flows. With -y, it would ideally suffice to specify the path of the top module alone, set any appropriate search paths, and sv2v would output the entire converted design. Does this usage pattern seem reasonable?

Unlike other tools, sv2v would not implicitly search the working directory (-y .) by default, as this could break existing flows.

kauser-rl commented 1 year ago

A -y option will do it! Happy to test it if you want. With regards to breaking existing flows, as they won't be explicitly using -y on the command line, it should still be backwards compatible?

kauser-rl commented 1 year ago

Hi @zachjs - any update on when (if there is agreement on it) this option would be added?

zachjs commented 1 year ago

It may be a few weeks before I can begin work on this feature in earnest. Once I have a better timeline, I'll let you know.

kauser-rl commented 1 year ago

Thanks! I will eagerly wait for your update.

zachjs commented 1 year ago

I pushed an implementation of this feature as commit 882900c2ed94ad64179efb78c238cdb5d3718836 to branch libdir. Please let me know what you think!

zachjs commented 1 year ago

I have merged this into master as commit 51c90baf5ecbe5b33fe5e1dac31d80b7c1917f17. I am still very eager for your feedback!

kauser-rl commented 1 year ago

Sorry about the delay. Trying to get a release done. I'll get to this in a couple of weeks and let you know how it goes. It is on my TODO list!

kauser-rl commented 1 year ago

Do you mind creating a new release?

zachjs commented 1 year ago

I am interested in getting your feedback on the behavior before I generate an official release. This would ensure I don't commit to behavior that is confusing or not useful.

What aspect of the release eases your workflow? I can imagine any of the following being necessary: tag, GitHub release, build artifacts, or Hackage release. For example, I could make a v.test.0.0.0.1 tag for you. If that doesn't work, I can cut a release within the next couple weeks.

kauser-rl commented 1 year ago

I need to get our IT to install it on our compute cluster as a versioned module. A test tag is fine. The idea is that we can always identify on what version of tools we have qualified our design for reproducibility in the future.

zachjs commented 1 year ago

I pushed a test tag v0.test.1: https://github.com/zachjs/sv2v/releases/tag/v0.test.1. Thank you trying it out!

kauser-rl commented 1 year ago

Got the tag installed. It shows:

> sv2v --version
sv2v 0.0.10

When I run the test case I attached with this ticket, I don't see the expected output. Did I get the command wrong?

> sv2v -I `pwd` -y `pwd` -w adjacent top.sv
mod_with_type.sv: unfinished conditional directives: 1

Note sure what unfinished conditional directives: 1 means.

zachjs commented 1 year ago

That error refers to an unmatched ifdef or similar when the end of a file is reached. I will look into making that message more helpful.

zachjs commented 1 year ago

I just pushed a change to make that error message more useful as commit a05659cd06b7da57fd5758f4eba011dc8517dc4d and tag v0.test.2. There may well be an issue with sv2v, so please let me know what you find!

kauser-rl commented 1 year ago

Fixed the unmatched ifdef error. I'm able to get the correct output (pasted below). However I have a small request. It appears that that unique modules are created inline in the same file. It would be nice to have an option to split them out into their own files with the name of the file matching the module name. Currently we have a flow that relies on module names matching file names.

mod_with_type.v

module mod_with_type_17C7C (
    type_i,
    sel_o
);
    input wire [2:0] type_i;
    output wire sel_o;
    parameter TYPE_W = 2;
    wire [TYPE_W - 1:0] mask;
    generate
        if (TYPE_W == 2) begin : g_typea
            assign mask = 2'b10;
        end
        else begin : g_typeb
            assign mask = 3'b101;
        end
    endgenerate
    assign sel_o = |(type_i & mask);
endmodule
module mod_with_type_3D46F (
    type_i,
    sel_o
);
    input wire [1:0] type_i;
    output wire sel_o;
    parameter TYPE_W = 2;
    wire [TYPE_W - 1:0] mask;
    generate
        if (TYPE_W == 2) begin : g_typea
            assign mask = 2'b10;
        end
        else begin : g_typeb
            assign mask = 3'b101;
        end
    endgenerate
    assign sel_o = |(type_i & mask);
endmodule
zachjs commented 1 year ago

What if sv2v supported something like --write some/out/dir/, where it would automatically split the output into one module per file written into that directory? The directory structure of the input files would be ignored. Would this be a useful alternative to --write adjacent?

kauser-rl commented 1 year ago

That would be lovely! Is it quick to add? We are aiming to release end of this week.

zachjs commented 1 year ago

I've pushed a version of the above feature as 04d65bb3881b911e7b71d8ac86b4e74d356f5cfe and test tag v0.test.3. There are few limitations:

Please let me know what you think of this new feature!

I'm inclined to think that this behavior is what I should have done for --write adjacent from the beginning, but I think it's too late to remove that feature.

kauser-rl commented 1 year ago

I don't think we will have any issues with your limitations. Let me get the version installed and test it. Thanks for quickly turning around.

kauser-rl commented 1 year ago

It works as expected. Thanks! Can we get a release tag please?

zachjs commented 1 year ago

Great to hear that it's working for you! I have made a release: v0.0.11. I think this was overdue. Please let me know if you run into any problems!

I also removed the test tags (v0.test.1, etc.) from earlier. I can restore them if necessary.

For my own reference and in case there is any interest, I've documented below what I did to make this release. I may try to script some of this in the future.

  1. make && make test
  2. Update sv2v.cabal and CHANGELOG.md to reference the new version.
  3. git commit -a -m "release v0.0.11" && git push origin master
  4. git tag -a v0.0.11 HEAD -m "Release v0.0.11" && git push origin v0.0.11
  5. stack sdist and upload the Hackage candidate.
  6. Create the GitHub release with the reformatted notes from the changelog.
  7. Publish the Hackage release.
  8. Ensure the GitHub Actions workflow created the release artifacts.
zachjs commented 12 months ago

@kauser-rl Is there anything more to work on for this issue?

kauser-rl commented 12 months ago

Nothing more. All good.