veripool / verilog-mode

Verilog-Mode for Emacs with Indentation, Hightlighting and AUTOs. Master repository for pushing to GNU, verilog.com and veripool.org.
http://veripool.org/verilog-mode
GNU General Public License v3.0
262 stars 91 forks source link

SystemVerilog attributes in module declaration are getting modified when expanding AUTOs #1884

Closed cswmeta closed 2 weeks ago

cswmeta commented 2 weeks ago

We have recently adopted SystemVerilog attributes as a way of annotating RTL with additional information.

If the attributes are placed inside the module header then Verilog mode adds a comma after each attribute. Here is an example:

module top
(

  (* period=5.0 *)
  input logic clk,

  (* asynchronous=true *)
  input logic rst_n,

  (* attribute1="hello" *)
  /*AUTOINPUT("^a_")*/
  /*AUTOOUTPUT("^a_")*/

  /*AUTOINPUT("^z_")*/
  /*AUTOOUTPUT("^z_")*/

  /*AUTOOUTPUT*/
);

  /*AUTOLOGIC*/

  module_a i_module_a
    (/*AUTOINST*/);

  module_b i_module_b
    (/*AUTOINST*/);

endmodule : top

module module_a
(
  input  logic        clk,
  input  logic        rst_n,

  input  logic [7:0]  a_data,
  input  logic        a_valid,
  output logic        a_ready,

  output logic [7:0]  b_data,
  output logic        b_valid,
  input  logic        b_ready
);
endmodule : module_a

module module_b
(
  input  logic        clk,
  input  logic        rst_n,

  input  logic [7:0]  b_data,
  input  logic        b_valid,
  output logic        b_ready,

  output logic [7:0]  z_data,
  output logic        z_valid,
  input  logic        z_ready
);
endmodule : module_b

See below for what top looks like after expanding the AUTOs. Notice the comma that appears after ( attribute1="hello" ). This comma is NOT removed if I remove the AUTOs with verilog-delete-auto.

module top
(

  (* period=5.0 *)
  input logic clk,

  (* asynchronous=true *)
  input logic rst_n,

  (* attribute1="hello" *),
  /*AUTOINPUT("^a_")*/
  // Beginning of automatic inputs (from unused autoinst inputs)
  input logic [7:0]     a_data,
  input logic           a_valid,
  // End of automatics
  /*AUTOOUTPUT("^a_")*/
  // Beginning of automatic outputs (from unused autoinst outputs)
  output logic          a_ready,
  // End of automatics

  /*AUTOINPUT("^z_")*/
  // Beginning of automatic inputs (from unused autoinst inputs)
  input logic           z_ready,
  // End of automatics
  /*AUTOOUTPUT("^z_")*/
  // Beginning of automatic outputs (from unused autoinst outputs)
  output logic [7:0]    z_data,
  output logic          z_valid
  // End of automatics

);

  /*AUTOLOGIC*/
  // Beginning of automatic wires (for undeclared instantiated-module outputs)
  logic [7:0]           b_data;
  logic                 b_ready;
  logic                 b_valid;
  // End of automatics

  module_a i_module_a
    (/*AUTOINST*/
     // Outputs
     .a_ready                           (a_ready),
     .b_data                            (b_data[7:0]),
     .b_valid                           (b_valid),
     // Inputs
     .clk                               (clk),
     .rst_n                             (rst_n),
     .a_data                            (a_data[7:0]),
     .a_valid                           (a_valid),
     .b_ready                           (b_ready));

  module_b i_module_b
    (/*AUTOINST*/
     // Outputs
     .b_ready                           (b_ready),
     .z_data                            (z_data[7:0]),
     .z_valid                           (z_valid),
     // Inputs
     .clk                               (clk),
     .rst_n                             (rst_n),
     .b_data                            (b_data[7:0]),
     .b_valid                           (b_valid),
     .z_ready                           (z_ready));

endmodule : top

Is there anything I can do to prevent the comma from being inserted?

wsnyder commented 2 weeks ago

Thanks for the good test, easy fix. You'll need to manually add/remove "," before that attribute if the input list changes to non-empty or empty.

cswmeta commented 2 weeks ago

I'm not sure that workaround is OK. If I manually remove the comma before the attribute then the SystemVerilog syntax for attributes is violated. The attribute needs to come immediate before the port declaration so the comma would cause a syntax problem.

I have another workaround... manually declare the first port that follows the attribute. Then expand AUTOs won't insert the comma.

Would be nice if verilog-mode would recognize this and handle the comma correctly. Though I have no LISP experience, I imagine it may be complex to update the code, so I'm OK with the workaround.

wsnyder commented 2 weeks ago

It's already fixed, see link above.

cswmeta commented 2 weeks ago

That was fast! Thanks!