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

SystemVerilog cast on input ports causes signal to be ignored #1526

Closed veripoolbot closed 3 years ago

veripoolbot commented 4 years ago

Author Name: Udi Finkelstein Original Redmine Issue: 1526 from https://www.veripool.org

Original Assignee: Wilson Snyder (@wsnyder)


In the following minimal code example, "x" is promoted to output although it's used as both input and output.

Commenting line 22 and uncommenting line 21 solves this issue, showing that the SV casting is the issue.

module t;
/*AUTOINPUT*/
/*AUTOOUTPUT*/
// Beginning of automatic outputs (from unused autoinst outputs)
output          x;          // From t2 of t2.v
// End of automatics
/*AUTOWIRE*/
wire [3:0][27:0]x;
typedef wire [3:0][27:0]t;
/*t2  AUTO_TEMPLATE  (
  .x(x),
  //.x(t'(x)),
)*/
t2 #(// Parameters
.N(4)
)
t2 (/*AUTOINST*/
     // Outputs
     .x                 (x));            // Templated
/*t3  AUTO_TEMPLATE  (
  //.x(x),
  .x(t'(x)),
)*/
t3 #(// Parameters
.N(4)
)
t3 (/*AUTOINST*/
     // Inputs
     .x                 (t'(x)));        // Templated
endmodule

module t2 #(
parameter N=4
) (
  output [N-1:0][27:0]x
);
endmodule

module t3 #(
parameter N=4
) (
  input [N-1:0][27:0]x
);
endmodule

I'm well aware of the fact that in this example "x" is even generated without dimensions, but at the moment this bothers me less because I declare the wire by myself. The real code where I ran into this uses partial indexing of a multidimensional reg, and I understand from past issues that this isn't supported anyhow.

All I want is just to stop it from being generated by AUTOOUTPUT. I know there are workarounds for disabling ports (dead `ifdefs etc.) but I don't want to add this unnecessarily to the code when it's Verilog-mode's fault.

Also, if I change line 21 to ".x(x[][])," the port is declared as

output [N-1:0] [27:0]   x;          // From t2 of t2.v

Instead of using the assigned parameter value (4).

veripoolbot commented 4 years ago

Original Redmine Comment Author Name: Wilson Snyder (@wsnyder) Original Date: 2019-09-26T12:55:21Z


Thanks for the good test case.

Believe I've fixed it, see git and verilog-mode-2019-09-26-ef55eb6-vpo, please give it a try on your larger design.

veripoolbot commented 4 years ago

Original Redmine Comment Author Name: Udi Finkelstein Original Date: 2019-09-26T13:50:12Z


Thanks!

That solved the issue on hand (on my full code), but it caused a new bug :-(

module t;
/*AUTOINPUT*/
/*AUTOOUTPUT*/
// Beginning of automatic outputs (from unused autoinst outputs)
output          x;          // From t2 of t2.v
// End of automatics
/*AUTOWIRE*/
wire [3:0][27:0]x;
/*t2  AUTO_TEMPLATE  (
  .x((x)),
)*/
t2 #(// Parameters
.N(4)
)
t2 (/*AUTOINST*/
     // Outputs
     .x                 ((x)));          // Templated

wire [27:0]a;
assign a = x[0][27:0];
wire [27:0]b;
assign b = x[1][27:0];

endmodule

module t2 #(
parameter N=4
) (
  output [N-1:0][27:0]x
);
endmodule

Using the previous (Sep 5) version, x is correctly detected and not inferred as output.

Using the new Sep 26 version, x is not detected correctly and inferred as output (as quoted above).

Even for the Sep 5 version, I had to put an extra parenthesis in the template to get it to work, otherwise 'x' would turn into an output as well.

veripoolbot commented 4 years ago

Original Redmine Comment Author Name: Wilson Snyder (@wsnyder) Original Date: 2019-09-26T19:00:14Z


In your example as I understand it, "x" is an output, and is not used as an input (always's are ignored) and so should correctly be in the autooutput list. It looks like you were relying on a bug in the older version. If you don't want x in the output list you might want to use verilog-auto-ignore-concat.

veripoolbot commented 4 years ago

Original Redmine Comment Author Name: Udi Finkelstein Original Date: 2019-09-27T05:10:10Z


Are you implying that referencing a signal within your module is not enough to keep it internal? Does it has to be an explicit input of another module?

I went over the available documentation, and it seems this is not clearly described. I've been using a different internal tool from my previous workplace in the past, where merely using a signal value within the module is enough to preclude it from the module's outputs, and so this seemd natural to me.

veripoolbot commented 4 years ago

Original Redmine Comment Author Name: Wilson Snyder (@wsnyder) Original Date: 2019-09-27T12:26:52Z


Are you implying that referencing a signal within your module is not enough to keep it internal?

Correct. Autooutput was originally for modules which just had submodules and it's too late to change the convention.

[[Verilog-mode-Help#verilog-auto-output]]

"Make output statements for any output signal from an /AUTOINST/ that isn't an input to another AUTOINST.

udif commented 3 years ago

Coming back to this issue after someone in my company stumbled on it.

Given the following synthetic example:

module t(
/*AUTOOUTPUT*/
);
/*AUTOWIRE*/
/*t2  AUTO_TEMPLATE  (
  .x((x)),
)*/
t2
t2_inst (/*AUTOINST*/
);
endmodule

module t2 (
  output x
);
endmodule

Using an old version that predates the ef55eb626b4989d665ef5b6a9fafdec6d66828be commit (the one fixing this issue), the example about would yield:

module t(
/*AUTOOUTPUT*/
);
/*AUTOWIRE*/
/*t2  AUTO_TEMPLATE  (
  .x((x)),
)*/
t2
t2_inst (/*AUTOINST*/
     // Outputs
     .x             ((x)));          // Templated

endmodule

module t2 (
  output x
);
endmodule

We were using ((...)) on t2_inst to hide it from the upper level AUTOOUTPUT. After the ef55eb626b4989d665ef5b6a9fafdec6d66828be commit, the result is:

module t(
/*AUTOOUTPUT*/
// Beginning of automatic outputs (from unused autoinst outputs)
output          x           // From t2_inst of t2.v
// End of automatics
);
/*AUTOWIRE*/
/*t2  AUTO_TEMPLATE  (
  .x((x)),
)*/
t2
t2_inst (/*AUTOINST*/
     // Outputs
     .x             ((x)));          // Templated

endmodule

module t2 (
  output x
);
endmodule

x is now propagated to the module t output.
It was a well-known fact for us that we can use ((...)) to hide signals from AUTO*, and out codebase is full of that. I have now searched for this in the documentation but couldn't find it (I'm pretty sure I saw this somewhere else). Are your comments above (almost 2 years ago...) imply that any ((...)) behavior was purely a bug and not an intended feature?

wsnyder commented 3 years ago

((x)) was a bug and not intended to be used. ({x}) that is with curly braces is a documented feature if you set verilog-auto-ignore-concat.

udif commented 3 years ago

I've tried verilog-auto-ignore-concat, but for my case it only makes things worse - Signals that had instances with input & output within concatenation (actual multi-signal concatenations, not those added to hide signals) that used to be AUTOWIRE are now becoming AUTOOUTPUT because they also used to have a single non concatenated instance.

I began thinking about adding a mode to track non-instance usage (so that RHS usage would cancel outputs and LHS use would cancel inputs), but the more I think about this, the more complex it is: I originally considered only searching for = and <= and track LHS and RHS by counting pairs of (), {}, [] on each side of the assignment symbol, but then I realized:

  1. <= is also used for comparison
  2. I won't be tracking RHS usage within statements such as if (), case (), for(), etc. Overall, it seems to be too complex to handle without a proper parser.

Maybe we can add a new mode where concatenation is considered only if there are no commas, so that ({x}) is really ignored, but ({x,y}) is not. ( As you may need to use ({x,y}) due to the design, but ({x}) is always intentional).

wsnyder commented 3 years ago

Maybe {{..}}? If you'd like to make a pull for that please feel free.

firefoxtc commented 3 years ago

hi. and thank you for the support in advance. you have a great tool that we love.

i am Tomer and i work with Udi (the one that started this thread)

we are now upgrading from a very old version of this tool, and the only thing that gives us trouble is the way to manually exclude ports that one side is in an auto_template and the other not (an always block or other non-auto generated way)

until now we used: .signal_name ((inst_name)), // the double round brackets prevented the signal from appearing in the auto input/output.

from the thread above i understand that this was an old bug that was fixed.

what i did not understand is:

  1. what is the correct way to do this
  2. we have found by trail and error that using round brackets does do the trick, is this another bug, or a feature that will stay working?

using ({x}) with verilog-auto-ignore-concat does not work well for us, since the usage of {} in verilog is for concatenation, and we use it even in auto_template quite a lot.

again, thanks for the support and quick answers.

wsnyder commented 3 years ago

Looking further, setting verilog-auto-ignore-concat will also cause (( to be ignored. I don't understand the comment about wanting {} to work however, as the older versions that ignored (('s would I thought also have been ignoring ({, That is verilog-auto-ignore-concat should make it similar to the older version.

firefoxtc commented 3 years ago

we cannot use ignore-concat since we need the auto_template to NOT ignore concatenations.

i still did not understand how i can declare a port to be ignored by the auto_input/output? without using {}

firefoxtc commented 3 years ago

hi wsnyder.

can you please help us with our problem? thanks again.

wsnyder commented 3 years ago

What about if we add a new /*AUTO_NOEXPAND*/ to the appropriate lines (that is within the template)?

firefoxtc commented 3 years ago

that would be great. any way to methodology define it, is good for me.

when can we expect a feature like this?

firefoxtc commented 3 years ago

and thank you for the reply

wsnyder commented 3 years ago

Would you be able to try a patch to add it? Otherwise I'll try to get to it next weekish.

firefoxtc commented 3 years ago

a patch is also good to make sure it works for us

wsnyder commented 3 years ago

Sorry, I meant would you be willing to try to make a patch yourself? Otherwise I'll try to get to it next weekish.

firefoxtc commented 3 years ago

i dont think i know how... when is the next weekish?

firefoxtc commented 3 years ago

when you say weekish, you mean next week or the one following?

firefoxtc commented 3 years ago

hi.

did you get the chance to look into adding this feature?

thanks

wsnyder commented 3 years ago

Sorry haven't gotten to it yet, still near the top of the queue, but the queue grows ;).

firefoxtc commented 3 years ago

i have been thinking that adding /AUTO_NOEXPAND/ within the auto_template might be a bit problematic since the auto_template is a comment it self.

having said that, any way you see fit, which you can implement should be fine with me. but it has to be within the auto_template.

thanks

firefoxtc commented 3 years ago

hi again.

we would like to upgrade to the new version next week. if you could please let me know if you will get to implementing this feature this week or not,

thanks

wsnyder commented 3 years ago

Please give the new AUTONOHOOKUP a try

/*t2  AUTO_TEMPLATE  (
  .x((x)),  // AUTONOHOOKUP
)*/
firefoxtc commented 3 years ago

we are now checking the version. it looks working, but i will do a more thorough check and update you,

thanks for your time and effort.

firefoxtc commented 3 years ago

we have tested this version. it works very well.

thank you for the effort.

if you can release an official revision with this update.