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
Labels
area: scheduling Issue involves scheduling/ordering of events area: wrong runtime result Issue involves an incorrect runtine result from Verilated model effort: days Expect this issue to require roughly days of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished

Comments

@veripoolbot
Copy link
Contributor


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


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


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


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


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


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


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


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


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


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


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


Not currently being worked on.

@veripoolbot veripoolbot added area: scheduling Issue involves scheduling/ordering of events area: wrong runtime result Issue involves an incorrect runtine result from Verilated model effort: days Expect this issue to require roughly days of invested effort to resolve status: blocked Issue is waiting for another bug, when other bug is fixed, then goes to 'status: assigned' labels Dec 22, 2019
@wsnyder wsnyder added resolution: wontfix Closed; work won't continue on an issue or pull request resolution: abandoned Closed; not enough information or otherwise never finished and removed status: blocked Issue is waiting for another bug, when other bug is fixed, then goes to 'status: assigned' resolution: wontfix Closed; work won't continue on an issue or pull request labels May 2, 2020
@wsnyder
Copy link
Member

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 as completed May 2, 2020
tgorochowik pushed a commit to antmicro/verilator that referenced this issue Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: scheduling Issue involves scheduling/ordering of events area: wrong runtime result Issue involves an incorrect runtine result from Verilated model effort: days Expect this issue to require roughly days of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished
Projects
None yet
Development

No branches or pull requests

2 participants