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

assert in write_cxxrtl #2540

Closed
galibert opened this issue Jan 14, 2021 · 2 comments · Fixed by #2871
Closed

assert in write_cxxrtl #2540

galibert opened this issue Jan 14, 2021 · 2 comments · Fixed by #2871
Labels
bug fix pending PR with a fix is pending

Comments

@galibert
Copy link
Contributor

m68k.v is the result of sv2v on fx68k

Do:
read -vlog95 m68k.v
hierarchy -top fx68k
write_cxxrtl m68k.cc

The write_cxxrtl step asserts in dump_sigchunk with a WireType::UNUSED that is not lhs.
ERROR: Assert `is_lhs' failed in backends/cxxrtl/cxxrtl_backend.cc:897.

m68k.zip

@galibert
Copy link
Contributor Author

mem.zip
Missing mem files

@Lunaphied
Copy link
Contributor

Lunaphied commented Jan 14, 2021

I've looked at this a bit and the issue appears to be that during write_cxxrtl it detects the wire as unused at optimization level -O5 or above. Those allow "public" wires (iirc this is Yosys slang for "named in the design but not an output), to be marked as LOCAL which means they're not exposed outside the eval() method for CXXRTL.

This later leads to the wire in question (in this case nDecoder.pcRelDbl) being marked as UNUSED because it ends up being able to be totally optimized out of the design (not 100% sure about the details on how that optimization takes place, but I'm assuming it's valid and that the tree of wires it connects to eventually terminates as unused).

Because the wire is UNUSED, this means that nothing should assign to it ever leading to the reason for the failing assert to exist. However, during -g4 (the highest debug info setting), debug info is created for all public signals in the design, including those that are optimized out. In order to do this it actually generates a secondary eval() method that includes such unoptimized signals in the design, running through the passes but with for_debug = true.

This shouldn't fail because theoretically there's a separate WireType mapping for the wires in debug mode, where UNUSED should be mapped to something else. Unfortunately, this doesn't occur, leading a wire in the debug version of the design that hasn't had it's type changed for eval_debug. This happens because not only do public debug visible wires have to be reintroduced, so does the entire tree of downstream connections.

The debug code basically re-runs the liveness check used before, but allowing OUTLINE (public wires visible only for debug) to be considered live as well, then assigns debug specific types to wires that are normally optimized out, but are used in the computation of other public signals.

I think the actual issue is the use of || instead of && here

if (live_wires[wire].empty() || debug_live_wires[wire].empty()) {

Since some wires will be in one set but not the other, and that check actually tests for the wire being unused in both sets. Patching that seems to resolve the issue and I'll submit a PR.

Lunaphied added a commit to Lunaphied/yosys that referenced this issue Jan 14, 2021
@whitequark whitequark added bug fix pending PR with a fix is pending labels Jan 25, 2021
whitequark added a commit to whitequark/yosys that referenced this issue Jul 16, 2021
Public wires may alias buffered internal wires, so keep BUFFERED
wires in debug information even if they are private. Debug items are
only created for public wires, so this does not otherwise affect how
debug information is emitted.

Fixes YosysHQ#2540.
Fixes YosysHQ#2841.
whitequark added a commit to whitequark/yosys that referenced this issue Jul 16, 2021
Public wires may alias buffered internal wires, so keep BUFFERED
wires in debug information even if they are private. Debug items are
only created for public wires, so this does not otherwise affect how
debug information is emitted.

Fixes YosysHQ#2540.
Fixes YosysHQ#2841.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix pending PR with a fix is pending
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants