Handle delayed signals in timing checks as assignments

[mole99]

Since timing checks are currently not supported in Icarus Verilog, they are ignored. This is an issue for some models:

Both $setuphold and $recrem have support for negative timing checks. For this to work, delayed versions of the reference and data signals must be created. The model has access to these delayed signals. The current situation is that these signals are never assigned.

This PR does what is proposed under 15.5.4 Option behavior in the standard. When timing checks are disabled (or in the case of Icarus Verilog not yet supported) the delayed reference and data signals become copies of the original reference and data signals.

For example, with the following timing check:

$setuphold(posedge sig1, negedge sig2 , 0:0:0 , 0:0:0 , notifier , cond1 , cond2 , del_sig1 , del_sig2 ) ;

Internally the simulator does the following:

assign del_sig1 = sig1;
assign del_sig2 = sig2;

Two tests were added to the regression suite:

• timing_check_delayed_signals - This test checks what this PR implements
• timing_check_syntax - This test makes sure that all timing checks can still be parsed

[mole99]

@caryr It would be great if you could have a look at what I have already.

Especially the following topics, as I wasn't sure / had some issues with them:

• I implemented the base class PTimingCheck from which all timing checks inherit (for now only PRecRem and PSetupHold because not more is needed for the assignments). Later on we can add the other timing checks, along with their implementation. Is this a good approach?

• The way how I parse the optional arguments is not optimal. Bison warns me about shift/reduce conflicts. Do you know a better way to do this?

• What are the functions in pform_dump for? Are they for debugging? I tried to run iverilog with -d elaborate, but I got none of my output.

• I still need support for vectors in the timing check assignments, e.g.

  $setuphold(posedge sig1[1], negedge sig2[3] , 0:0:0 , 0:0:0 , notifier , cond1 , cond2 , del_sig1[0] , del_sig2[4] ) ;
  

  If you have any tips for that, please tell me.

[caryr]

Having a base class seems appropriate and is what I was planning for my implementation.

A shift/reduce warning is an indication you have added alternate paths and the parser doesn't know which to take. This can cause invalid parsing and would need to be resolved before this can be included. This often happens when you try to follow the BNF too closely. It is a guide, but you often have to implement things slightly differently to make the parser happy.

The pform dump is for debugging the early stage of the compiler. It can be enabled, but as I remember it's a bit more involved and I always have to look the details up myself since I seldom need to debug that early in the compiler. I sometimes use the pform dump directly to make sure the thing I am working on is correct, but seldomly use it in its entirety.

To be honest I was expecting you to just check if the delayed signals were used and if so call the appropriate assignment pform function to handle this along with changing the warning message regarding the delayed signals. So basically just minor parser changes versus the extensive changes you did make. This goes back to my initial comment. "Look at how assignments are handled."

[mole99]

Hi Cary,

I see, I got things mixed up. A classical case of miscommunication :) For the SDF INTERCONNECT feature I will make sure to properly state my plans for the implementation.

Is it alright if I continue with this implementation? Eventually, when the timing checks get implemented, it may already be convenient to have them in pform.

I will see to it that that I fix the shift/reduce warnings during parsing.

[caryr]

Yes, continue and fix the shift/reduce issues. It would also be nice to have the basic pform dump working, but I can add that when I get everything finalized. For the interconnect you have most of the implementation anyways.

[mole99]

Hi Cary, I have improved the parsing of the timing checks now, so that no shift/reduce warnings are reported anymore.
It actually wasn't that simple, because the timing checks are one of the few statements where you can just omit arguments and specify the following ones:

$setuphold ( posedge CLK , posedge D , 0:0:0 , 0:0:0 , notifier ,  ,  , CLK_delayed , D_delayed ) ;

I think I've solved this quite nicely with a structure that contains the optional arguments and is passed through the rules.

I also tried creating the assignments directly in parse.y with pform_make_pgassign_list as you suggested. However, that seems to work only partially: The delayed signals are driven, but multiple assignments are created due to potentially multiple timing checks. As a result, the output of the flip-flop becomes x. Is there an easy way to tell if an assignment has already been made to prevent a second driver? If not, then I would say that the current solution is sufficient for now.

Therefore I would say this PR is ready for review.

[caryr]

Yes, there is the rule that all drivers with the same input collapse to a single driver, but I would not expect there to be different driving signals connected to the same delayed point. Do you have a simple example?

I will look at the details later. I would have expected the required "," between optional elements to resolve any parsing ambiguity, but there is likely some subtlety I am missing.

[mole99]

Yes, for example the dfrtp flip-flop in SYK130 has the following timing checks:

$recrem ( posedge RESET_B , posedge CLK , 0:0:0 , 0:0:0 , notifier , AWAKE , AWAKE , RESET_B_delayed , CLK_delayed ) ;
$setuphold ( posedge CLK , posedge D , 0:0:0 , 0:0:0 , notifier , COND0 , COND0 , CLK_delayed , D_delayed ) ;
$setuphold ( posedge CLK , negedge D , 0:0:0 , 0:0:0 , notifier , COND0 , COND0 , CLK_delayed , D_delayed ) ;

Therefore CLK_delayed is assigned three times and D_delayed two times.

With pform_make_pgassign_list the resulting simulation looks like this:

Whereas it looks like this with this PR:

[mole99]

Regarding the parsing: There is no problem anymore. I just meant that it was not straight-forward to implement because arguments can be omitted and the total arguments vary in length. Or at least it was not straight-forward for me ;)

[caryr]

So the difference is because with the multiple drivers each is updated at a slightly different inter time step point so there is a zero width region of contention which generates an X that breaks the system. You would have needed to look at the driving source and only created an assign once and then reused the output of the assign for each successive element. The standard talks about this in excessive detail. A simple associated array based on the scope of the input drive could have been used to keep track of this. You could have also omitted the assign and just connected everything to the input signal, but the assign is safer since if something corrupts the output signal you would not expect it to corrupt the input signal and that is what would happen if they used the same net.

[mole99]

So the difference is because with the multiple drivers each is updated at a slightly different inter time step point so there is a zero width region of contention which generates an X that breaks the system.

Very interesting, I would have never guessed that this is the reason, but it makes sense, as the output gets only then corrupted when the input changes.

You would have needed to look at the driving source and only created an assign once and then reused the output of the assign for each successive element. The standard talks about this in excessive detail. A simple associated array based on the scope of the input drive could have been used to keep track of this.

Yes that's what I asked with: "Is there an easy way to tell if an assignment has already been made to prevent a second driver?"

But during parse time the only way would be to check all PGAssign, right? Because they have not been elaborated yet. But this does not feel like a good approach.

You could have also omitted the assign and just connected everything to the input signal, but the assign is safer since if something corrupts the output signal you would not expect it to corrupt the input signal and that is what would happen if they used the same net.

I think this is what I implemented in this PR. I have connected the signals via connect(sig->pin(0), sig_delayed->pin(0)) and thus connecting a signal twice still counts as it being connected once to the net, correct?

This PR is so far working for its use case. I would propose to merge this as is and create an Issue to document that the output may corrupt the input signal. I feel like I should start working on SDF INTERCONNECT soon, as I do not know yet how much time this will occupy. Do you agree?

[larsclausen]

Some small comments mainly around making more use of C++11 features

[mole99]

Thank you for your review @larsclausen. I appreciate it!

[caryr]

There are some formatting issue that do not match our normal coding style, but more importantly all the checks for non-NULL before deleting is not required.

"The delete operator may be applied only to a pointer returned by new or to the nullptr. Applying
delete to the nullptr has no effect."

[mole99]

Thanks, I knew that felt wrong somehow! I removed all checks for non-NULL before deleting.

Could you maybe point out where there are formatting issues? I'm not too familiar with this formatting style.

It seems like the first level of indentation always uses (six) spaces, but then on subsequent levels one would use tabs? But then the number of spaces varies, or the formatting is inconsistent throughout the codebase.
Then the files used for parsing seem to have completely different formatting.

I would be open to setting up an automatic formatter after my GSoC work, for example clang-format.

This would simplify things, especially for newcomers to the project, and you could focus on the really important things. Also modern IDEs / text editors often pay attention to the coding style automatically.

We would set the coding style rules in .clang-format and then run the formatter on the whole codebase or on parts of it. Even the Linux kernel uses clang-format.

Should I open a discussion in Discussions around this topic for more input?

[caryr]

In general it is six spaces or a combination of tabs and spaces to give a multiple of six. There are exceptions for example, case statements where the label and code blocks do not both get an indent of six each. I'm going to merge this to get it in and will work on the formatting on the side since I'm going to be working in this area anyways.

[mole99]

Thanks for the explanation! Now with the case statement it makes sense.