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

Missing initial positive edge when using --x-initial-edge #678

Closed
veripoolbot opened this issue Sep 17, 2013 · 11 comments
Closed

Missing initial positive edge when using --x-initial-edge #678

veripoolbot opened this issue Sep 17, 2013 · 11 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Sep 17, 2013


Author Name: Charlie Brej
Original Redmine Issue: 678 from https://www.veripool.org


If --x-initial-edge is set, the __Vclklast of clock values is set to 1 to trigger the negedge action. If the initialization routine sets the value to 1, then neither the positive nor negative edge of that signal are triggered.

The bug was introduced in "Fix ordering of clock enables with delayed assigns, #�" patch (b277bc8).

Attached file demonstrates the fault.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 18, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-18T12:55:06Z


Hi Charlie,

Thanks for the report. I did the work on this some time ago and I'll investigate. I'll try to get this sorted (or at least understand why it broke) later today or tomorrow morning.

Jeremy

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 19, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-19T16:35:27Z


Hi Charlie,

I think the old behavior was wrong, and Bug 613 fixed it (very much as an unexpected side effect, which I still don't understand properly). If you define a signal with "initial", then it isn't X to start with, so it should not be affected by --x-initial-edge.

I've tested against an event driven simulator (Icarus), and its behaviour is consistent with this. Do you see the same behavior with NC or VCS?

However...

It used to work for you, and now it is broken. I need to work out if there is a way to restore the behavior you want. You want initial to happen not at the very beginning, but after one cycle of checking for clock edges. Can you confirm this is the semantics you need?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 23, 2013


Original Redmine Comment
Author Name: Charlie Brej
Original Date: 2013-09-23T17:57:37Z


Hi Jeremy

So the issue we have is we would like to do the following

reg trig;
initial
begin
 if ($test$plusargs("enable_trig"))
     trig <= 0;
 else
     trig <= 1;
end

always @(posedge trig)
begin
 $display("Feature trig enabled");
end

Except Verilator disregards the non-blocking write in the initial block. This sequence does work in Icarus.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 23, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-23T19:03:07Z


Hi Charlie,

OK that makes sense with a delayed assignment. I'll need to do a bit of digging into how Verilator transforms delayed assignments inside initial blocks to see what's possible. Give me a day or two.

Jeremy

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 24, 2013


Original Redmine Comment
Author Name: Charlie Brej
Original Date: 2013-09-24T10:39:32Z


Our current workaround is rather crude:

--- a/src/V3Order.cpp
+++ b/src/V3Order.cpp
@@ -632,6 +632,7 @@ private:
                 OrderVarVertex* varVxp = newVarUserVertex(varscp, WV_STD);
                 varVxp->isClock(true);
                 new OrderEdge(&m_graph, varVxp, m_activeSenVxp, WEIGHT_MEDIUM);
+               varVxp->isDelayed(true);
             } else {
                 if (!m_logicVxp) nodep->v3fatalSrc("Var ref not under a logic block\n");
                 // What new directions is this used


@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 25, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-25T08:55:09Z


Hi Charlie,

That's a fascinating approach. Marking all variable references in sensitivities as delayed. I need to think this through, to see how it will affect the overally ordering algorithm. Have you run a regression test to see what wider effects this has.

The approach I am investigating was whether the change from Bug 613 could be restricted, so it did not apply to delayed variables in initial blocks.

Jeremy

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 29, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-29T18:00:47Z


Hi Charlie,

This is a little tricky, because by the time we get to @V3Order.cpp@, we don't know if an initial block used delayed assignments. However it is fairly easy to leave a marker around for that. We then need to add initial blocks to the order tree in @V3Order.cpp@, so we can then detect them in the @AstNodeVarRef@ visitor and if it is a delayed assignment in an initial block add a suitable vertex and edge.

I've done all this, and it mostly works. See branch issue-678 at https://github.com/jeremybennett/verilator.

But...

This change breaks @processMove()@, and we don't get the correct @cfunc@ tree generated for initial blocks. The upshot is that you get dupliate declarations of @_initial__TOP@ functions in the generated code. I have partly ameliorated this, by eliminating unused initial logic vertices in @iterateNewStmt()@, but this is really just covering up the problem. There is still one regression error, @t_initial_dlyass.pl@ caused by this patch, but it is symptomatic of the problem.

It's getting late tonight, but I hope either you or Wilson might see what the final part of the patch is. I'll take another look in a day or two.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 30, 2013


Original Redmine Comment
Author Name: Charlie Brej
Original Date: 2013-09-30T16:07:56Z


Hi, so Just testing this patch on our designs we are getting multiple definitions of _initial__TOP:

void VTEST_DESIGN_DSM::_initial__TOP(VTEST_DESIGN_DSM__Syms* __restrict vlSymsp) {
     VL_DEBUG_IF(VL_PRINTF("    VTEST_DESIGN_DSM::_initial__TOP\n"); );
     VTEST_DESIGN_DSM* __restrict vlTOPp VL_ATTR_UNUSED = vlSymsp->TOPp;
     // Body
     // INITIAL at ../file.sv:581
...
void VTEST_DESIGN_DSM::_initial__TOP__1(VTEST_DESIGN_DSM__Syms* __restrict vlSymsp) {
     VL_DEBUG_IF(VL_PRINTF("    VTEST_DESIGN_DSM::_initial__TOP__1\n"); );
     VTEST_DESIGN_DSM* __restrict vlTOPp VL_ATTR_UNUSED = vlSymsp->TOPp;
     // Body
     // INITIAL at ../some_file.sv:1510
...
void VTEST_DESIGN_DSM::_initial__TOP(VTEST_DESIGN_DSM__Syms* __restrict vlSymsp) {
     VL_DEBUG_IF(VL_PRINTF("    VTEST_DESIGN_DSM::_initial__TOP\n"); );
     VTEST_DESIGN_DSM* __restrict vlTOPp VL_ATTR_UNUSED = vlSymsp->TOPp;
     // Body
     vlTOPp->_initial__TOP__1(vlSymsp);
}


I will investigate a little, but any help would be appreciated.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 30, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-09-30T17:57:18Z


Hi Charlie,

OK - so we need to fix that processMove() problem. I'll continue to work on it.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 18, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-18T00:06:30Z


Not currently being worked on.

@wsnyder
Copy link
Member

@wsnyder wsnyder commented May 2, 2020

No longer being worked & the general need for x-initial-edge will eventually be replaced by better event scheduling.

@wsnyder wsnyder closed this May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.