Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TriState code generation uses poorly supported Verilog-2001 port aliases #327

Open
thoughtpolice opened this issue Feb 25, 2021 · 7 comments

Comments

@thoughtpolice
Copy link
Contributor

thoughtpolice commented Feb 25, 2021

import TriState::*;

interface T;
  interface Inout#(bit) v;
endinterface

(* synthesize *)
module t(T);
  let t <- mkTriState(False, 'b0);
  interface Inout v = t.io;
endmodule

bsc -verilog -u T.bsv gives:

module t(CLK,
	RST_N,

	.v(t$IO));
  input  CLK;
  input  RST_N;

  inout  t$IO;

  // ports of submodule t
  wire t$IO;

  // submodule t
  TriState #(.width(32'd1)) t(.I(1'b0), .OE(1'd0), .O(), .IO(t$IO));
endmodule  // t

Looks like there's some simple bug that causes the names to be generated improperly. See below; this is a standard, but uncommon feature.

@thoughtpolice
Copy link
Contributor Author

This is actually a general problem with all code generation invoving inout ports (i.e. I believe the TriState.bs module has a correct binding; it's actually a flaw in the code generator.) I'm seeing how easy a fix is...

@quark17
Copy link
Collaborator

quark17 commented Feb 25, 2021

Can you explain what you think is wrong with that Verilog?

I notice that you've instantiated a submodule with instance name t that is the same name as the module t. These are in different scopes, so I don't believe there is a conflict, but it's confusing as a reader. I assume that's not related to your concern?

If you're referring to the dot notation in the port list, that is valid Verilog. However, we have noticed that some tools do not like it. In the util directory, there is a script called basicinout.pl which can be called with the Verilog file as argument. It will overwrite that file with a new version, that changes code like this:

module ... ( ... .v(t$IO) ... );
   ...
   inout  t$IO;
   ...
   TriState ... t( ... .IO(t$IO) ... );
   ...
endmodule

into this:

module ... ( ... v ... );
   ...
   inout  v;
   ...
   TriState ... t( ... .IO(v) ... );
   ...
endmodule

The reason we use the first form is because there is no other way to express designs where multiple inout ports share the same signal. In your case, and probably in most designs, there are no shared inout ports, and so the simpler form can be used. (We might want to improve BSC to output this simpler form, when possible, so that there'd be no need for the script. I don't recall if there's a reason why that's hard to do.)

@thoughtpolice
Copy link
Contributor Author

I've never seen this syntax before (and it frankly looks wholly wrong; I'd only have expected such syntax in SystemVerilog perhaps; I guess that's just another thing Verilog can surprise you with...) and my synthesis tools don't support it. I did look into a fix for this last night for the simpler form, but didn't get anywhere unfortunately.

@thoughtpolice
Copy link
Contributor Author

For the record, after doing some diving, this syntax was apparently introduced in Verilog 2001; see IEEE-1364-2001 section 12.3.3 Port declarations.

@quark17
Copy link
Collaborator

quark17 commented Feb 25, 2021

It's been around since Verilog 2001 and was in 2005 -- I can't say whether newer standards have removed it. In the IEEE 1364 standards, it is described in chapter 12 on "Hierarchical structures", in 12.3 "Ports". It appears first in the syntax grammar table:

port ::=
  [ port_expression ]
  | . port_identifier ( [ port_expression ] )

and it appears in the examples:

module same_port (.a(i), .b(i));
// Name 'i' is declared inside the module as an inout port.
// Names 'a' and 'b' are defined for port connections.

I believe that if you have two ports sharing the same inout, that is the only way you can write the module.

The examples also include:

module complex_ports ({c,d}, .e(f));
// Nets {c,d} receive the first port bits.
// Name 'f' is declared inside the module.
// Name 'e' is defined outside the module.
// Can't use named port connections of first port.

module split_ports (a[7:4], a[3:0]);
// First port is upper 4 bits of 'a'.
// Second port is lower 4 bits of 'a'.
// Can't use named port connections because
// of part-select port 'a'.

module renamed_concat (.a({b,c}), f, .g(h[1]));
// Names 'b', 'c', 'f', 'h' are defined inside the module.
// Names 'a', 'f', 'g' are defined for port connections.
// Can use named port connections.

module same_input (a,a);
input a; // This is legal. The inputs are ored together.

@thoughtpolice thoughtpolice changed the title TriState code generation is broken TriState code generation uses poorly supported Verilog-2005 port aliases Feb 25, 2021
@thoughtpolice thoughtpolice changed the title TriState code generation uses poorly supported Verilog-2005 port aliases TriState code generation uses poorly supported Verilog-2001 port aliases Feb 25, 2021
@thoughtpolice
Copy link
Contributor Author

As an aside, I've filed a bug for this in Yosys, and hopefully this simply strengthens my case for Verilog-2001-or-later support in e.g. #294. :)

@remexre
Copy link

remexre commented Sep 24, 2024

Would it make sense to print a warning if this syntax is used in the generated Verilog when the -v95 flag is passed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants