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

Triggering initial edge from X #570

Closed
veripoolbot opened this issue Oct 18, 2012 · 5 comments
Closed

Triggering initial edge from X #570

veripoolbot opened this issue Oct 18, 2012 · 5 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Oct 18, 2012


Author Name: Jeremy Bennett (@jeremybennett)
Original Redmine Issue: 570 from https://www.veripool.org
Original Date: 2012-10-18
Original Assignee: Jeremy Bennett (@jeremybennett)


Event driven simulators will generally trigger an edge on a transition from X to 1 (@Posedge@) or X to 0 (@negedge@). Thus the following code, where @rst_n@ is uninitialized would set @res_n@ to @1'b1@ when @rst_n@ is first set to zero:

 reg  res_n = 1'b0;

 always @(negedge rst_n) begin
    if (rst_n == 1'b0) begin
       res_n <= 1'b1;
    end
 end

In Verilator, by default, uninitialized clocks are given a value of zero, so the above @Always@ block would not trigger.

Whilst it is not good design practice, there are some designs that rely on X -> 0 triggering a @negedge@, particularly in reset sequences.

This patch adds a new option, @--x-initial-edge@, to Verilator which will cause initial edges from X to trigger @Posedge@ or @negedge@ as appropriate. This matches the behaviour of event driven simulators (and reportedly commercial cycle-accurate simulators).

The patch works by setting initial values of @clklast@ variables in the constructor not to zero, but to the inverse of the associated clock variable.

I would appreciate review of the approach I use. I only test for this in @emitVarResets@ (in @V3EmitC.cpp@) in the case of 1-bit wide variables for which @zeroit@ is true, but I think this is always the case for clocks.

The @clklast@ variable is constructed in @getCreateLastClk@ (in @V3Clock.cpp@) using the @shortname@ of the clock variable, which removes any @PVT@ strings from local variables. I couldn't see why this is done, but it makes it impossible to reconstruct the name of the clock variable from the name of the clklast variable. So I have extended @Astvar@ with a new field (@m_lastBasep@) to record this variable. I have also updated the constructors to set this and provided an accessor function. This field is NULL, except in the case where a @clklast@ variable is created, when it points to the associated clock variable.

I have updated the documentation accordingly. I noticed some inconsistencies in the use of @--@ or @-@ in naming long options to Verilator, which I have tidied up (I hope). I've also added some clarifications to the Internals manual, based on my experience with this patch.

Two new regression tests, check this behaviour works correctly when the option is set, and does not work when the option is not set.

Please pull the patch from the initial-edge branch at git@github.com:jeremybennett/verilator.git.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 30, 2012


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2012-10-30T10:30:25Z


Basically makes sense. A few minor things:

I'm also not sure why I used the shortName, I think it's better if you change that back, and not use lastBasep, if that also works. The problem with having additional pointers is future optimizations might change variables around and make the pointer stale. If you don't see another alternative then lastBasep then the "AstVar::broken()" method should test that the target of lastBasep is still valid.

Also while it works for your one bit case, using ~0 will cause problems with other widths and is not properly MSB clean (extra top bits are set, which violates the assumption elsewhere that these bits are zero), so it is better to use the correct invert functions or just set it to the appropriately formed constant directly. BTW if I was to do it again I would put as little special code in V3EmitC as possible, so probably would have a AstVar::ctorValuep() that says how to construct value. If you want to make that, set it in your code, then use it in V3EmitC that would be ideal. (If so, I'd suggest AstVar::ctorValuep() be tested in V3EmitC, and used for construction if non-NULL, else you can fall back the existing ugly code.)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 31, 2012


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2012-10-31T20:17:30Z


Thanks for all the suggestions. I've taken out lastBasep and used name() instead of shortName() and the two specific tests still work.

I couldn't find a specific function for inverting (I looked in verilated.h), so instead I generate

(1 & (~ (IData)(varname)));

I haven't put in the ctorValuep() change. I think if I understand you correctly it should have the logic from V3EmitC, so that EmitCImp::emitVarResets() could just call it instead to decide what to output. Is this correct?

I'll run regression on this to check nothing breaks and then resubmit tomorrow.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 1, 2012


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2012-11-01T13:32:16Z


New version of the patch, which no longer requires lastBasep, and which correctly preserves bits when inverting is now available. This passes regression test. Please pull from the initial-edge branch at git@github.com:jeremybennett/verilator.git.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 2, 2012


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2012-11-02T23:57:54Z


My comments about inversion were confusing; I made it closer to your original version.

Thanks for the patch & modifications. Fixed in git towards 3.842.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 4, 2012


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2012-11-04T00:25:44Z


In 3.842.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.