yosys frontend cannot handle delays with a specified unit

[ted-xie]

Steps to reproduce the issue

After unzipping the attached zip archive: yosys -p 'read_verilog case.v' -> succeeds. yosys -p 'read_verilog case_bad.v' -> fails.

My yosys version:

$ yosys -version
Yosys 0.9+4052 (git sha1 0ccc7229, clang 11.0.1-2 -fPIC -Os)

Expected behavior

I would expect that at least in the read_verilog stage, q <= #1 d would be treated the same way as q <= #1ns d. read_verilog should at least pass if I specify a unit on the delay in a nonblocking assignment.

Actual behavior

read_verilog fails because it expects another token. See error message below.

Parsing Verilog input from `case_bad.v' to AST representation.
case_bad.v:8: ERROR: syntax error, unexpected TOK_ID

[ted-xie]

yosys_delay_case.zip

I somehow messed up attaching the test case. See above attachment.

[Ravenslofty]

The delays are not synthesisable constructs, so picking between something that gets ignored and something that doesn't compile doesn't make much difference; whether you choose a delay of 1 clock cycle or 1 nanosecond, you're getting no delay at all.

[udif]

First, I think #1 is not 1 clock cycle but 1 in the current timescale. Second, this notation is very common, not to add real delay to the design but to make it easier to look at your waveforms and know for sure what is the signal value at clock rise time. so ignoring this or failing does make a difference. In the codebase I work on my day job, we simply define:

`define D #1

For simulation, and

`define D

For synthesis.

[Ravenslofty]

First, I think #1 is not 1 clock cycle but 1 in the current timescale.

Okay, yes, I looked at the Verilog standard and you are correct, I apologise.

Second, this notation is very common, not to add real delay to the design but to make it easier to look at your waveforms and know for sure what is the signal value at clock rise time. so ignoring this or failing does make a difference.

I don't quite understand this, but either way, undefining it in synthesis still means Yosys can ignore it.

[ted-xie]

I don't need the 1ns delay to synthesize (I am completely aware that it is not synthesizable). As @udif mentioned, this is a simulation construct meant to enforce a sim-only delay on the clk-to-Q latency. Other synthesis tools such as Xilinx Vivado, Intel Quartus, and Synopsys Design Compiler all support the syntax, so Yosys should too. Furthermore, these delays are part of the Verilog LRM, so the frontend should support them.

undefining it in synthesis still means Yosys can ignore it.

It is better to reduce the number of ifdefs in synthesis code because they can lead to costly one-liner mistakes.

[zachjs]

I will take a pass at adding support for time literals in delays. For simplicity, the implementation will likely be a bit more permissive than the official grammar by allowing number literals in general rather than just unsigned and fixed-point numbers.