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
247 stars 90 forks source link

Improving the AUTOINST handling of signed ports #1750

Open fnJeff opened 2 years ago

fnJeff commented 2 years ago

The original issue I brought up several years ago was that AUTOINST would sometimes include bit slices for ports that were signed and cause tools like DC to give warnings about signed to unsigned conversions.

As you can see below, I already have the verilog-auto-inst-vector set to unsigned. This works for one very particular case, but it would be nice if it worked more generally. I will try and explain with testcases.

First, here is the submodule for debugging.

module debug_signed_ports
   (input  logic signed [3:0] a,
    input  logic signed [3:0] b,
    output logic signed [4:0] c
    );
   assign c = a + b;
endmodule

Here is the one case that seems to work. You can see that a, b, and c ports do not add the bit slices that would cause the warnings.

module top;
   logic signed [3:0] a;
   logic signed [3:0] b;
   logic signed [4:0] c;

   /*AUTOLOGIC*/

   /*debug_signed_ports AUTO_TEMPLATE (
    );*/
   debug_signed_ports debug_signed_ports
      (/*AUTOINST*/
       // Outputs
       .c                               (c),
       // Inputs
       .a                               (a),
       .b                               (b));

endmodule

First case that doesn't work is if you try and use AUTOLOGIC to declare the variable for the output port. Here you can see that AUTOLOGIC correctly declared the signed variable, but AUTOINST stopped recognizing it as a signed port and added the bit slice.

module top;
   logic signed [3:0] a;
   logic signed [3:0] b;
//   logic signed [4:0] c;

   /*AUTOLOGIC*/
   // Beginning of automatic wires (for undeclared instantiated-module outputs)
   logic signed [4:0]   c;                      // From debug_signed_ports of debug_signed_ports.v
   // End of automatics

   /*debug_signed_ports AUTO_TEMPLATE (
    );*/
   debug_signed_ports debug_signed_ports
      (/*AUTOINST*/
       // Outputs
       .c                               (c[4:0]),
       // Inputs
       .a                               (a),
       .b                               (b));

endmodule

Second case that doesn't work is using a template to change the name of the variables connected to the ports and using AUTOLOGIC for the output variable. Now you can see that all of them are no longer recognized as signed and the bit slices are included. The first thing you might suggest is just to remove the []. This is a trivial example, but there are cases I have where patterns in the AUTO_TEMPLATE can be used for signals matching both signed and unsigned ports based on naming conventions. So it would really be nicer if AUTOINST was able to determine a signed input from the submodule port declarations and not depend only on local wires of identical names.

module top;
   logic signed [3:0] a_int;
   logic signed [3:0] b_int;
//   logic signed [4:0] c_int;

   /*AUTOLOGIC*/
   // Beginning of automatic wires (for undeclared instantiated-module outputs)
   logic signed [4:0]   c_int;                  // From debug_signed_ports of debug_signed_ports.v
   // End of automatics

   /*debug_signed_ports AUTO_TEMPLATE (
       .\([abc]\)                      (\1_int[]),
    );*/
   debug_signed_ports debug_signed_ports
      (/*AUTOINST*/
       // Outputs
       .c                               (c_int[4:0]),            // Templated
       // Inputs
       .a                               (a_int[3:0]),            // Templated
       .b                               (b_int[3:0]));            // Templated

endmodule

Third case that doesn't work is extra weird because it highlights a possible name clash between submodule port naming and higher level variable naming. If you happen to have higher level variables with the same name as a submodule port that are signed and the same width, AUTOINST then uses the information of the variable matching the submodule port name to determine that the port is signed even if the variable is not connected to or related in any way to the AUTOINST port. That feels dangerous. You can see in this example that I add a declaration for a and c, but those are not used or connected to the submodule. Yet AUTOINST removes the bit slices from the connected wires.

module top;
   logic signed [3:0] a_int;
   logic signed [3:0] b_int;
//   logic signed [4:0] c_int;

   logic signed [3:0] a;
   logic signed [4:0] c;

   /*AUTOLOGIC*/
   // Beginning of automatic wires (for undeclared instantiated-module outputs)
   logic signed         c_int;                  // From debug_signed_ports of debug_signed_ports.v
   // End of automatics

   /*debug_signed_ports AUTO_TEMPLATE (
       .\([abc]\)                      (\1_int[]),
    );*/
   debug_signed_ports debug_signed_ports
      (/*AUTOINST*/
       // Outputs
       .c                               (c_int),                 // Templated
       // Inputs
       .a                               (a_int),                 // Templated
       .b                               (b_int[3:0]));            // Templated

endmodule

I understand this is a bit complicated to solve. It seems AUTOINST has to know it's output is signed and then check what is connected to (also signed and same full bit width) it after AUTO_TEMPLATE and AUTOLOGIC have done their work if necessary.

Debug info:

Emacs : GNU Emacs 27.1 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 3.14.13) of 2020-12-17 Package: verilog-mode v2021-10-14-797711e-vpo-GNU

current state: (setq verilog-active-low-regexp nil verilog-after-save-font-hook nil verilog-align-ifelse nil verilog-assignment-delay "" verilog-auto-arg-sort nil verilog-auto-declare-nettype nil verilog-auto-delete-trailing-whitespace nil verilog-auto-endcomments nil verilog-auto-hook nil verilog-auto-ignore-concat nil verilog-auto-indent-on-newline t verilog-auto-inout-ignore-regexp nil verilog-auto-input-ignore-regexp nil verilog-auto-inst-column 40 verilog-auto-inst-dot-name nil verilog-auto-inst-interfaced-ports nil verilog-auto-inst-param-value nil verilog-auto-inst-sort nil verilog-auto-inst-template-numbers nil verilog-auto-inst-vector 'unsigned verilog-auto-lineup 'declarations verilog-auto-newline nil verilog-auto-output-ignore-regexp nil verilog-auto-read-includes nil verilog-auto-reset-blocking-in-non t verilog-auto-reset-widths t verilog-auto-save-policy nil verilog-auto-sense-defines-constant nil verilog-auto-sense-include-inputs nil verilog-auto-star-expand t verilog-auto-star-save nil verilog-auto-template-warn-unused nil verilog-auto-tieoff-declaration "wire" verilog-auto-tieoff-ignore-regexp nil verilog-auto-unused-ignore-regexp nil verilog-auto-wire-type "logic" verilog-before-auto-hook nil verilog-before-delete-auto-hook nil verilog-before-getopt-flags-hook nil verilog-before-save-font-hook nil verilog-cache-enabled t verilog-case-fold t verilog-case-indent 3 verilog-cexp-indent 3 verilog-compiler "verilog " verilog-coverage "echo 'No verilog-coverage set, see \"M-x describe-variable verilog-coverage\"'" verilog-delete-auto-hook nil verilog-getopt-flags-hook nil verilog-highlight-grouping-keywords t verilog-highlight-includes t verilog-highlight-modules t verilog-highlight-translate-off nil verilog-indent-begin-after-if t verilog-indent-declaration-macros nil verilog-indent-level 3 verilog-indent-level-behavioral 3 verilog-indent-level-declaration 3 verilog-indent-level-directive 1 verilog-indent-level-module 3 verilog-indent-lists t verilog-library-directories '(".") verilog-library-extensions '(".v" ".va" ".sv") verilog-library-files nil verilog-library-flags '("") verilog-linter "echo 'No verilog-linter set, see \"M-x describe-variable verilog-linter\"'" verilog-minimum-comment-distance 10 verilog-mode-hook 'verilog-set-compile-command verilog-mode-release-emacs t verilog-mode-version "2021-10-14-797711e-vpo-GNU" verilog-preprocessor "verilator -E FLAGS FILE" verilog-simulator "echo 'No verilog-simulator set, see \"M-x describe-variable verilog-simulator\"'" verilog-tab-always-indent t verilog-tab-to-comment nil verilog-typedef-regexp "^\(t.+\|sm[^_]+\)$" verilog-warn-fatal nil )

wsnyder commented 2 years ago

Thanks for the good write up. I looked at this further.

I understand this is a bit complicated to solve. It seems AUTOINST has to know it's output is signed and then check what is connected to (also signed and same full bit width) it after AUTO_TEMPLATE and AUTOLOGIC have done their work if necessary.

That's a correct summary. Basically at present it does not analyze what the upper connection is at all, and I'm unlikely to add that capability as it will be complicated and hard to get right.

The "verilog-auto-inst-vector 'unsigned" hack was added under the presumption that the connection was not templated, which accounts for some of the bad behavior you note. Perhaps if there's any template the "verilog-auto-inst-vector 'unsigned" should simply be ignored?