Extend "delay" expressions to handle pair and triplet, i.e. rise, fall and turn-off

[TimRudy]

• Per LRM A.2.2.3
• Not for synthesis, just for Verilog acceptance by front-end

Resolves #2565

[TimRudy]

Thanks Prof whitequark! Well that was pretty easy. Do you know if propagating new non_opt_delay formats -> through delay -> all through the language, such as simple_behavioral_stmt, behavioral_stmt, wire_decl, assign_stmt, cell_stmt will be transparent and not affect the system?

[whitequark]

@zachjs Could you please take a look at this PR? I don't understand the relevant parts of the LRM well enough.

[eddiehung]

Welcome @TimRudy! Would you be able to add a quick test to (a) demonstrate its usage, and (b) make sure it doesn't get broken in the future?

Maybe inside tests/verilog -- adapt an existing test there?

[zachjs]

Relevant portions of the SV grammar: (repeat, #, (, ), :, and , are terminals)

delay3 ::= #delay_value | #(mintypmax_expression[,mintypmax_expression[,mintypmax_expression]])
delay2 ::= #delay_value | #(mintypmax_expression[,mintypmax_expression])
delay_value
  ::= unsigned_number
  | real_number
  | ps_identifier
  | time_literal
  | 1step
delay_control
  ::= # delay_value 
  | # ( mintypmax_expression )
mintypmax_expression
  ::= expression 
  | expression : expression : expression
delay_or_event_control
  ::= delay_control 
  | event_control
  
| repeat ( expression ) event_control 

So you can have something like

module top;
  wire o, a, b;
  and #(1:2:3, 4:5:6) and_gate (o, a, b);
  wire #(1:2:3, 4:5:6, 7:8:9) x;
  assign x = o;
endmodule

• delay_control and delay3 can both be used in net declarations and continuous assignments
• delay_control and event_control are both allowed to guard procedural statements, as is the more obscure (to me) cycle_delay
• delay_or_event_control is used for procedural assignments
• delay2 is used in primitives (and, nor, etc.) gate declarations

Both with and without these additions, there are false-positive and false-negative parses with the current grammar. That said, these delays are naturally dropped for synthesis, so it may not be worth worrying about the false-positives. I have no objections to merging this PR for now and revising our grammar for delays down the line. I can also take a stab at this in the near term if desired.

[TimRudy]

My enthusiasm for open source dropped off a cliff as cygwin and building was not smooth, I was on StackOverflow which is not a good karma for a weekend passtime. (However after a little segmentation violation scare in an ABC file - solution, try again - C/C++ compilation is like riding a bike? - there is truly a lot of building done now.) To get yosys to finish I need a fresh build of iverilog from Git - but the iverilog build is giving me the bigger problems. ... OK, I did have version 10.1 installed and I seem to have found an 11 of it for Windows at http://bleyer.org/icarus, that's gotta work, it's recent.

Back to you about those tests soon

[udif]

@TimRudy Have you considered using WSL2?

[TimRudy]

Hi guys,
I've done make test and overcame a problem which was: my Windows path happened to have a space in it. Yes! This provides a chance to improve Yosys (platform builds). Should I open an issue?

It comes down to using cygpath to make those Windows short paths, e.g. looks like:

Parsing Verilog input from `/cygdrive/c/LEVEL1~1/Software/yosys/share/techmap.v'

In autotest.sh I put:

Needs an issue, I guess: What is best way to do these kind of things in platform-independent .sh files?
With ifndef CYGWIN or ifdef WIN32 in supporting .c files?

Any comments on that welcome.

[TimRudy]

Then another issue I'll need to bring up: For me to use gcc (in cygwin context), apparently I needed to make targeted change in Makefile:

What do you think of that? Trying to help :-) - and willing to do some legwork to improve things in the corners so convenient for strange, odd users

[zachjs]

@TimRudy I don't think I'm qualified to comment on the build/script issues mentioned above. However, I'm interested in moving forward with this change. If you're currently blocked, I'd also be happy to take a stab at supporting these and other delay variants and add some test coverage.

[TimRudy]

Thanks - My next step is to clone a file like bug2037.ys in verilog test

logger -expect-no-warnings
read_verilog <<EOT

and write a number of examples of delays in the grammer - starting with only what we would call legitimate verilog-compliant

There are non-compliant constructs that you referred to above, I understand, and if tests were written for those they would pass, but would indicate Yosys is allowing weird delays, right? I will do the above basics, some tonight, OK?

[zachjs]

I think it's fine to not worry about coverage for the "negative" cases for the time being. I can add coverage in a future PR once the grammar includes more of the positive cases I described above.

[TimRudy]

Yeah I can tell you're interested in tweaking the grammar, so it breaks out the delay specs a bit... That would be cool, I thought about it but will leave that stuff to you. Can you give an example of a positive case so I understand specifically?

[TimRudy]

Sorry Zach, I couldn't run tests in a timely way on my machine, so will check in again on the weekend

[TimRudy]

I'll push some tests tomorrow, I got the tests to run & pass

[TimRudy]

Notes, updated:

I didn't uncover every corner of the grammar

• I tried time_literal like 10us and I tried 1step as a token and those are not accepted
• Does repeat ( expression ) event_control come into our delay considerations? I tried repeat (3) @(posedge clk); but I guess that's not for synthesis, it syntax errored at @
• I heard a delay could work in an initializer, example at line 49 of risefall file, line 68 of mintypmax file: wire [15:0] add0_res = #(15 : 20 : 25) a + b; (but it was not accepted)

Finally, I didn't do SV test - Cheers

[zachjs]

Thanks for adding test coverage and highlighting some of the other cases we should look into in the future. Would you mind squashing so the tests are added in the same commit as the grammar changes? Otherwise, LGTM.

[TimRudy]

More missing expressions added to grammar, may help even more people avoid syntax errors. If that's OK, let me know and I'll send the squash commit

[zachjs]

LGTM, let's get this squashed and I'll merge it!

[zachjs]

Can you squash these commits before we merge? This can help make log/blame more useful on popular files like these.

[eddiehung]

I see that "squash and merge" is the default setting now. Is there a need to do it on the source branch too?

[TimRudy]

Does my head commit look like a squash or like a merge? Tell me if it's good to go, thanks Eddie

[whitequark]

I see that "squash and merge" is the default setting now.

It's only the default for you (it's per repo/user combination).

Is there a need to do it on the source branch too?

You can only squash all commits with the GitHub merge button, but sometimes it makes sense to squash some. (Not sure which category this PR is in.)

[eddiehung]

I see that "squash and merge" is the default setting now.

It's only the default for you (it's per repo/user combination).

Ooh I didn't know that, it must have remembered my last setting. I would assume that it could be enforced at the repo-wide level if one wanted. I think it's the right thing to do now (despite being careless and not doing so in the past and getting my commit count up!)

Is there a need to do it on the source branch too?

You can only squash all commits with the GitHub merge button, but sometimes it makes sense to squash some. (Not sure which category this PR is in.)

I would say that when you ask a contributor to squash, they will typically squash all commits into one. Clicking the button could save that round trip.

[zachjs]

It is possible to individually enable or disable each of the three merge options (merge, squash, rebase) at the repo level. While squashing into a single commit is often appropriate, I don't think we should require squashing for all changes. I personally prefer rebase-only, but this would be a significant departure for this project. I also wouldn't mind using GitHub's squash for this and other changes, so long as we are comfortable with results.

[whitequark]

I agree with @zachjs here.

[zachjs]

Barring any objections, I will generally opt for squashing change sets like this one. Note that the author of the squashed commit will likely be the creator of the PR, so if a PR is opened on behalf of another author, we may want to manually squash instead. I also think using rebases rather than merge commits is sometimes preferred, so we may consider trying that out in the near-term and disabling the "standard merge" option if we like it long-term.