[sv-bc] Re: 1619 suggestions

Hi, Tom -

Thanks for the reply. I hope you don't mind, I have copied the entire 
sv-bc because others might have the same questions.

At 11:28 AM 12/3/2007, Alsop, Thomas R wrote:
>Hi Cliff,  some follow up questions, see comments below. My 
>apologies if I am just missing your point.  Perhaps some follow up 
>mail can flush this all out.  Thanks for you inputs, they are 
>appreciated.  -Tom
>
>// Cliff's notes concerning 1619
>//
>// (1) The top example shows my concern over added default ports used with .*
>// (2) The second example shows a proposed modification (albeit rather ugly)
>// (3) The third example shows how to handle this with the current 
>syntax and no
>// default ports
>// (4) The forth example shows how to `include a core model in an 
>existing model
>//---------------------------------------------------------------------------
>
>
>//---------------------------------------------------------------------------
>// IP Provider - New Model for accum module
>//---------------------------------------------------------------------------
>// IP Provider believes the better version of the model inverts the dataout
>// MSB when not reset and has added an invert-MSB (inv_msb) control input
>// and set it to 1 so that future use of the model will invert the dataout
>// MSB by default
>//---------------------------------------------------------------------------
>module accum (
>   output logic [7:0] dataout,
>   input        [7:0] datain,
>   input              clk, rst_n='1, inv_msb='1);
>
>   always @(posedge clk, negedge rst_n)
>     if (!rst_n) dataout <= '0;
>     else        dataout <= {inv_msb,7'b0}^datain; // New functionality
>endmodule
>
>I am not sure what you are implying by this code change.  I seems to 
>me that if an IP provider releases a new version of their code, it 
>must be backwards compatible, which implies that the inv_msb should 
>be set to '0.  They should understand the new implications of how 
>default ports work with .* if they are going to use this feature. 
>The enhancement to the code is then put into user documentation 
>telling them to set this port value to '1 if they want the new 
>functionality.  Any IP provider _must_ IMHO, be savvy enough to not 
>only ensure their code is backwards compatible but also document 
>these types of enhancements or users should find someone else.

Fair point. Perhaps I am just too concerned about the knowledge of IP 
providers. We agree that an IP provider SHOULD release a version with 
new ports that would be backward compatible, but there are a number 
of Verilog book writers and IP providers that should just be sued for 
mal practice. If IP providers do the right thing, your proposal 
should be perfectly fine. Unfortunately, the proposal does not 
guarantee that IP providers will do the right thing, and my example 
above is a potential hazardous case.

Like I tell people, Verilog is a language that allows you to hang 
yourself good if you don't know what you are doing while VHDL is a 
language that makes it hard to even build a scaffold.

This is a potentially useful enhancement that again makes it easier 
to hang yourself if you don't know what you are doing.

In recent years, I have been trying to tighten up the language with 
each new enhancement without going overboard.

>  // All other models as defined in Mantis 1619
>...
>
>module alu_accum4 (
>    output [15:0] dataout,
>    input   [7:0] ain, bin,
>    input   [2:0] opcode,
>    input         clk);
>    wire    [7:0] alu_out;
>
>    alu   alu   (.*, .zero());
>    // accum rst input is tied high AND inv_msb is now tied high
>    // (inv_msb port did not exist before - added by IP provider)
>    accum accum (.*, .dataout(dataout[7:0]), .datain(alu_out));
>    xtend xtend (.*, .dout(dataout[15:8]),   .din(alu_out[7]));
>endmodule
>
>//---------------------------------------------------------------------------
>
>// Alternative (Ugly) syntax - not much better or safer but does require
>// "responsible" action by the IP provider
>module accum (
>   output logic [7:0] dataout,
>   input        [7:0] datain,
>   input              clk,
>   // Shows that this port can be omitted in .* connection
>   input ()           rst_n='1,
>   // missing () shows that this port must be listed by .*
>   input              inv_msb='1);
>
>   always @(posedge clk, negedge rst_n)
>     if (!rst_n) dataout <= '0;
>     else        dataout <= {inv_msb,7'b0}^datain; // New functionality
>endmodule
>
>Wow, yeah, I think this is really stretching this new feature for a 
>case that should be understood well enough by IP providers in the 
>first place.  It adds complexity to an already complex feature and 
>is really only providing a safeguard for IP providers.  That said, 
>how is this going to guarantee that even this () gets used correctly 
>by the IP folks as opposed to just using the new feature as they should?

There are no guarantees that even this would fix the potential 
problems (which is why I am still leaning towards being the only 
no-vote). But with this type of syntax, an IP provider could not just 
say, "well, the user should always use a named port. My IP was never 
intended to be used without a named port." If an IP provider added () 
to the new port, users could charge the IP provider with mal practice 
if they changed the default with the added pin. If the IP provider 
did not add () to the pin, the end user would get an error about the 
port without a corresponding net declaration, so the user gets 
immediate feedback that something has changed.

>//---------------------------------------------------------------------------
>
>// Alternative instantiation without default ports
>
>module alu_accum4 (
>    output [15:0] dataout,
>    input   [7:0] ain, bin,
>    input   [2:0] opcode,
>    input         clk);
>    wire    [7:0] alu_out;
>
>    alu   alu   (.*, .zero());
>    accum accum (.*, .dataout(dataout[7:0]), .datain(alu_out), .rst_n('1),
>    `ifdef INV_MSB
>    .inv_msb('1));
>    `else
>    .inv_msb('0));
>    `endif
>
>    xtend xtend (.*, .dout(dataout[15:8]),   .din(alu_out[7]), .rst('0));
>endmodule
>
>I'm not sure what you are trying to show here.  I understand that 
>this is a way to set port values if you don't have default ports, 
>but the proposal is there so I don't have to do this.

Understood and agreed. Although the new notation is convenient, the 
old notation was not terrible. That was the intent of this example.

>//---------------------------------------------------------------------------
>
>// Alternative (currently legal) syntax for accum module
>// use config instance statement to select the correct accum
>// in the alu_accum4
>
>module accum (
>   output logic [7:0] dataout,
>   input        [7:0] datain,
>   input              clk,
>
>   assign rst_n='1;
>   assign inv_msb='1;
>
>But this doesn't even allow you to override these values upon 
>instantiation.  Sorry if I am just missing your point againJ  Please clarify.

True. The point is, you can maintain multiple copies of wrappers 
around a common generic core, allow all of the wrappers to have the 
same module name and then use config "instance" statements to choose 
which configuration you would like for each instance.

>   // module in module
>   `include "accum_core.v"
>endmodule
>
>// Separate file accum_core.v
>module accum_core (
>   output logic [7:0] dataout,
>   input        [7:0] datain,
>   input              clk,
>   input              rst_n,
>   input              inv_msb);
>
>   always @(posedge clk, negedge rst_n)
>     if (!rst_n) dataout <= '0;
>     else        dataout <= {inv_msb,7'b0}^datain; // New functionality
>endmodule
>
>//---------------------------------------------------------------------------

My concern is that with SystemVerilog-2005 we added a very nice 
instantiation capability using .* that enforced a whole set of very 
useful design checks inside of a very concise and useful enhancement. 
I can see the value of default port values but I also see the danger 
and although some engineers would take advantage of the ability, I 
believe most will not. On the other hand, with this enhancement in 
place, ignorant IP providers could cause significant damage to the 
simple-case designs.

I keep placing blame on IP providers but in fact, there is a whole 
group of self-taught SystemVerilog users that are going to abuse this 
capability as well.

I am probably the only dissenting vote, so vote me down and there 
will be no hard feelings. I just hope the usage of this feature is 
rare enough that only the knowledgeable will use it.

Regards - Cliff


----------------------------------------------------
Cliff Cummings - Sunburst Design, Inc.
14314 SW Allen Blvd., PMB 501, Beaverton, OR 97005
Phone: 503-641-8446 / FAX: 503-641-8486
cliffc@sunburst-design.com / www.sunburst-design.com
Expert Verilog, SystemVerilog, Synthesis and Verification Training


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.