make setimmediatevalue immediately set

[toddstrader]

See: #1024

Currently vpiInertialDelay prevents setimmediatevalue() from immediately setting a value. This attempts to fix that, but honestly raises more questions in my mind. Namely that the VHPI and FLI interfaces don't seem to have a notion analogous to vpiIntertialDelay. So I'm once again wondering if we really want that for the VPI? It seems odd that putting a value would be incongruous like that between the different GPI impls.

In any case, I'm positive that setimmediatevalue() should set immediately and test_assign_immediate attempts to lock that in.

[toddstrader]

So this breaks test_timing_triggers.test_writes_have_taken_effect_after_readwrite. I believe the timeline is:

• write_manually is started soon
• in_data is assigned via .value (this is deferred for _do_writes to handle later)
• write_manually starts up and awaits ReadWrite
• that causes _do_writes to actually put the value, but in the case of the VPI it is with vpiInertialDelay which means it won't actually be applied until the simulator's scheduler moves forward
• write_manually wakes up and assigns in_data with setimmediatevalue which with this PR is put with vpiNoDelay
• in_data is immediately updated
• we await ReadOnly()
• this causes the simulator's scheduler to march forward which causes the vpiInertialDelay put to happen
• the assert sees the .value value (3) instead of the setimmediatevalue one (2)

The test seems right to me and I think this PR makes setimmediatevalue via the VPI work as advertised. This leads me back to questioning vpiInertialDelay. Are we sure we want that? It seems like we've got two stages of assignment deferring which is what's causing this test to fail now:

• _do_writes()
• vpiInertialDelay

[marlonjames]

Add a test that creates a new task waiting on Edge(signal) and then call signal.setimmediatevalue() in another task, to make sure that works.
I would expect the new test to hit the icarus re-entrancy problem.

[toddstrader]

Thanks @marlonjames . Also, it's not just an icarus problem. Xcelium also gives me FATAL: We are calling up again.

[toddstrader]

I believe the latest commit addresses the reentrancy problem (at least it fixes my test for icarus and xcelium). I can split that off into another PR if desired or keep it here. Still need to dig into why test_writes_have_taken_effect_after_readwrite is busted . . .

[toddstrader]

Scratch that ^ it's issue_768_* I need to dig into more (for icarus). test_writes_have_taken_effect_after_readwrite is still failing, but is understood and explained above. Still curious if people think that the failure of test_writes_have_taken_effect_after_readwrite demonstrates that vpiInertialDelay is a problem.

[marlonjames]

It probably makes sense to move the re-entrancy handling to a different PR.
It shouldn't be recursive, it needs to handle a callbacks from other threads, and I think we need a little more discussion on which layer to queue the pending callbacks.

[toddstrader]

It probably makes sense to move the re-entrancy handling to a different PR.

Sounds good. I'd like to keep them combined for now because I need the setimmediatevalue change to test the reentrancy change and I need GH's CI to see all the simulator results. But I can break them up once this is all looking good.

The 768 tests are correctly applying the value to stream_in_data via setimmediatevalue, but once the Timer is awaited that value can no longer be observed. I've also noticed that if I await seemingly anything at the beginning of the test (e.g. ReadWrite, Timer) I don't run into this issue. And I only see this with Icarus. I've tried to reproduce this in a stand-alone test to see if it's an Icarus bug but I can't get the same behavior there. I've tried doing the vpiNoDelay assignment from a cbStartOfSimulation callback and from a systf function via initial but neither show this issue.

I'll see if I can figure out what the difference is, but wanted to post here to see if this sounded familiar to anyone.

[marlonjames]

It may have to do with which simulation region of the timestep the cbStartOfSimulation callback is executed.
If that occurs before whatever sets the initial value of stream_in_data, it would get overridden.

[ktbarrett]

We would definitely want to handle the immediate callback issue at the GPI implementation level (or even push the fix to the simulator if possible) and not in the Python wrapper. So I'm all for deferring that fix for another PR.

[toddstrader]

I lied. I can make the 768 issue happen with either cbStartOfSimulation or cbAfterDelay with a delay of zero (I believe this is what cocotb is doing for Icarus). I'll post a reproducer for Icarus, but we'll likely need a band-aid in the meantime.

[toddstrader]

Alright. So there's a bunch of things going on here.

Issue 768

I would posit that steveicarus/iverilog#1111 was the real problem with #768 all along. Let's see what the Icarus folks say and if / how quickly they can get a fix out. But I don't think we ever should have needed vpiInertialDelay to fix #768.

VPI reentrance

Protection against different threads calling back is a good call. We should protect access to the deferred list with a mutex.

Re: moving it to the GPI, I think I could put this in GpiValueCbHdl::run_callback() or if we're concerned about the more general problem, make GpiCbHdl::run_callback() handle the deferral problem and dispatch to a virtual actually_do_callback() method. I'm not sure I understand the objection to recursion. I could also code this up as a while loop. Personally I feel the recursive solution is easier to read but am am happy to refactor it in whatever way people think best.

test_writes_have_taken_effect_after_readwrite

I think when setimmediatevalue() actually sets immediately, this test shows that there is a problem with .value's double deferral. I would suggest that this indicates we shouldn't be using vpiInertialDelay at all. Thoughts?

[toddstrader]

how quickly they can get a fix out

Answer: zero time. It was already fixed. If I re-run the 768 tests against HEAD of Icarus with this change, they now pass.

[toddstrader]

we shouldn't be using vpiInertialDelay at all

I ran nox on this branch with both Icarus (HEAD) and Xcelium without vpiInertialDelay and everything passed. So I'm increasingly pro this idea.

[imphil]

how quickly they can get a fix out

Answer: zero time.

We're talking about setimmediate, after all.

[ktbarrett]

Putting values in VHPI and FLI are always inertial, so vpiInternalDelay is more consistent if the effect is visible to the user. Ideally it should not be. If we are going to jump on to no delay only, we need to test out a way of setting value immediately in the VHPI and FLI as well. I think the easiest way is to Force then immediately Release.

[ktbarrett]

I think the easiest way is to Force then immediately Release.

This doesn't work. Even Forces are inertial. Is there anything analagous to vpiNoDelay in the VHPI @marlonjames?

[ktbarrett]

After further investigation I'm leaning more towards not using vpiNoDelay and removing setimmediatevalue unless there is some path forward for consistent behavior.

Currently setimmediatevalue's only value is that because _write_calls is deferred to the beginning of ReadWrite and writes are inertial, setimmediatevalue sets are seen at the beginning of ReadWrite while normal writes are not. I think the current setimmediatevalue behavior should be the default, which we can accomplish along with a performance benefit by changing when the _write_calls are applied as mentioned in #1024 (comment).

This will also defer applications of Force so we get consistent behavior between VHDL and Verilog and also avoid the terrible re-entrant scheduler in Icarus and Xcelium.

[ktbarrett]

I'm actually fine with this approach. It just needs to pass :)

[toddstrader]

I'll revise this once #3875 lands. Really wish GitHub allowed stacked PRs . . .

[ktbarrett]

@toddstrader you could rebase onto my branch and just write "depends on #3875". We don't don't do merge commits here so, you might need to rebase if that other commit changes a bunch.