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

values in vcd are zero despite simulation's nonzero values #429

Closed
programmerjake opened this issue Jul 10, 2020 · 9 comments · Fixed by #430
Closed

values in vcd are zero despite simulation's nonzero values #429

programmerjake opened this issue Jul 10, 2020 · 9 comments · Fixed by #430

Comments

@programmerjake
Copy link
Contributor

@programmerjake programmerjake commented Jul 10, 2020

Still haven't narrowed this one down to a reproducer that's all that useful:
http://lists.libre-riscv.org/pipermail/libre-riscv-dev/2020-July/008506.html

used revisions:
nmigen : 30e2f91
ieee754fpu : 610b4a381e70f45f5684cc281398ce77fb5441fa
nmutil : 3853df675a1e1db24950945f66b076266a7da409
soc : caceb716e9417ed8731ef08b7b260a4c077186b2

ieee754fpu, nmutil, and soc can be cloned using:

for i in ieee754fpu nmutil soc; do
    git clone --recursive git://git.libre-riscv.org/$i.git
done

activate a python3 venv:

<path/to/venv>/bin/activate

in each of nmigen, ieee754fpu, and nmutil, run:

python setup.py develop

in soc run:

make install
python src/soc/fu/div/test/test_pipe_caller.py # will fail the tests

the generated vcd file is div_simulator.vcd in soc's top level folder.

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 10, 2020

values that should be nonzero because print calls produce the right values:
path in vcd: top.alu.pipe_start.setup_stage.ra

@whitequark whitequark added the bug label Jul 10, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

Can you bisect nmigen to find the culprit?

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 11, 2020

in case anyone's interested, here's the script I'm using to bisect it:
http://lists.libre-riscv.org/pipermail/libre-riscv-dev/2020-July/008532.html

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 11, 2020

Well, I got only partially useful results:

git bisect's output:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
ba1b4c2
14845af
d5d3bc4
102565f
6bfff25
369bc3e
75a4665
7fa4b34
8dd28fe
6c0530c
fd5ee54
416b253
76b3ef4
76c7e70
f24f1b0
126f0be
f115335
1fbd7f1
f5670a9
175c8a5
43da4e3
981e674
f50303c
7fca037
d4946b0
3a4576e
adbc947
23758e3
78c027f
7d25bd5
8f6eab0
20baea4
c9ac85a
9f731d1
e012e62
2efeb05
b0dbbb6
c202661
db4529a
39c3bac
c9030eb
5048c5e
cee43f0
659b0e8
d5c297a
9928b60
e435a21
399b8f9
d3d210e
8dacbbb
94faf49
25ce260
90e2a99
2606ee3
3c3cfd4
6d41756
We cannot bisect more!
bisect run cannot continue any more

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 11, 2020

so, it just skipped every commit between the first and last commits -- I skipped if it didn't get far enough to output a vcd file with the signal included.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 11, 2020

Okay, so bisect isn't useful here for some reason. I've tested general VCD writing with Minerva SoC and that works, so it's not completely broken.

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 13, 2020

I think I found the cause of the bug:

In the broken code on line 348 it writes the current state to the vcd right before the next state is committed to the current state:
https://github.com/nmigen/nmigen/blob/30e2f91176edcd1c8766c2cb11f413b9c77936b9/nmigen/sim/pysim.py#L339-L354

In the working code it writes the current state right after the state is committed:
https://github.com/nmigen/nmigen/blob/303ea18cb60567e45a755c6b6289a601f27d46e6/nmigen/back/pysim.py#L245-L255

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 13, 2020

I think I found the cause of the bug:

Thanks for the investigation! This is related to the way this code uses pending to speed up VCD writing (IIRC, writing all signals unconditionally took quite significant time). I'll fix this properly soon, but in the meantime, I believe this temporary fix should get your code working:

diff --git a/nmigen/sim/pysim.py b/nmigen/sim/pysim.py
index 8b0c054..c419cc4 100644
--- a/nmigen/sim/pysim.py
+++ b/nmigen/sim/pysim.py
@@ -255,7 +255,7 @@ class Simulator(SimulatorCore):
             for waveform_writer in self._waveform_writers:
                 for signal_state in self._state.pending:
                     waveform_writer.update(self._state.timeline.now,
-                        signal_state.signal, signal_state.curr)
+                        signal_state.signal, signal_state.next)
 
             # 2. commit: apply every queued signal change, waking up any waiting processes
             converged = self._state.commit()

@programmerjake
Copy link
Contributor Author

@programmerjake programmerjake commented Jul 13, 2020

I think I found the cause of the bug:

Thanks for the investigation! This is related to the way this code uses pending to speed up VCD writing (IIRC, writing all signals unconditionally took quite significant time). I'll fix this properly soon, but in the meantime, I believe this temporary fix should get your code working:

we ended up making the same change in parallel :)

whitequark pushed a commit that referenced this issue Jul 13, 2020
@whitequark whitequark reopened this Jul 13, 2020
@whitequark whitequark added this to the 0.3 milestone Aug 26, 2020
@whitequark whitequark closed this in c9fd000 Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants