UCSBarchlab / PyRTL

A collection of classes providing simple hardware specification, simulation, tracing, and testing suitable for teaching and research. Simplicity, usability, clarity, and extensibility are the overarching goals, rather than performance or optimization.
http://ucsbarchlab.github.io/PyRTL
BSD 3-Clause "New" or "Revised" License
252 stars 76 forks source link

Wires used but never driven #422

Open JulianKemmerer opened 2 years ago

JulianKemmerer commented 2 years ago

Hi there,

I am quite unsure if this issue stems from PyRTL itself and am looking for some feedback/suggestions.

I have a flow where I am doing timing analysis on VHDL code.

VHDL -> GHDL plugin for yosys -> yosys write_blif -> pyrtl.input_from_blif

PyRTL steps look like: processor_instances_0CLK_a3e7f19d_top.blif.zip

import pyrtl
pyrtl.reset_working_block()
print("Opening .blif...", flush=True)
f=open("processor_instances_0CLK_a3e7f19d_top.blif")
blif = f.read()
pyrtl.input_from_blif(blif)
#print("Optimizing...", flush=True)
#pyrtl.optimize(skip_sanity_check=True)
print("Computing FMAX...", flush=True)
timing = pyrtl.TimingAnalysis()
print("Fmax (MHz):", timing.max_freq(), flush=True)
Traceback (most recent call last):
  File "COPY_processor_instances_0CLK_a3e7f19d_top.py", line 10, in <module>
    timing = pyrtl.TimingAnalysis()
  File "/home/julian/.local/lib/python3.8/site-packages/pyrtl/analysis.py", line 165, in __init__
    self.block.sanity_check()
  File "/home/julian/.local/lib/python3.8/site-packages/pyrtl/core.py", line 557, in sanity_check
    raise PyrtlError('Wires used but never driven: %s \n\n %s' %
pyrtl.pyrtlexceptions.PyrtlError: Wires used but never driven: ['tmp1983138', 'tmp1082396', 'tmp1822303', 'tmp1983160', 'tmp1757923', 'tmp278173', 'tmp889418', 'tmp1114543', 'tmp1725667', 'tmp1050205', 'tmp85153', ' ... ALOT OF THESE

Previously I was happy with results from adding in those two commented out lines

print("Optimizing...", flush=True)
pyrtl.optimize(skip_sanity_check=True)

It was apparently doing what was needed to trim the unused/undriven? wires...

The cause for this issue is that pyrtl.optimize is taking a long time (and this is a ~smaller version of the design). I think it is doing more than the bare minimum I need it to regarding the Wires used but never driven tmp wires.

Maybe adding a skip_sanity_check=True option to TimingAnalysis could work?

Or I think the _remove_unlistened_nets step in optmize is all I need?

Open to any comments+suggestions on the whole flow using the tools

Thanks folks

timsherwood commented 2 years ago

Hi Julian,

Undriven wires are often because a wire was intended to be an input but it was coded as a simple wire. Basically it is a wire that is that is a dangling input. The function _remove_unlistened_nets removes the wires that are driven but never used (i.e. dangling outputs... which do appear during the process of optimization). To be honest, it is not obvious why optimization would remove them, unless they were some weird artifact of their generation (like they were always an input to an AND gate with a constant zero).... or perhaps they are part of some completely disconnected component (e.g. an adder instantiated but never used)? You could certainly play around with calling _remove_unused_wires and _remove_unlistened_nets. A couple more thoughts:

Does the .blif file have all of the inputs listed as ".inputs"?

Is there a really simple .blif (maybe generated in the same way but for simple logic gate) that has the same problem and we can dissect to figure out what is going on?

For that smaller input you might try running it with debug mode on, that way we can track back to where in the blif parser we are generated those wires which might (in turn) help us figure out the issue.

JulianKemmerer commented 2 years ago

So the original VHDL does indeed have places where constant propagation through, ex. AND gates, is likely expected to occur... But I thought constant propagation and other optimization steps would be done by yosys. Which made me wonder what optimizations PyRTL was finding/doing that likely could be better handled upstream...

I did not try to read the .blif. But I will try to create a smaller example that shows the issue.

Thanks for your time eh!

timsherwood commented 2 years ago

Yeah, that would be my expectation as well. We have only tested PyRTL with the Verilog->YoSys->Pyrtl path and have not tried VHDL at all -- could be something being generated differently there.

JulianKemmerer commented 2 years ago

This .blif shows the issue and is much smaller The VHDL is for an 16b accumulator which is some pretty ugly generate code and has like two levels of hierarchy around it with registers. top_265f.blif.txt

If I had to bet it's maybe whatever these undefined $undef looking connections are, ex.

.names $undef \36.read_pipe[113]
1 1

From the VHDL I can tell you read_pipe is a VHDL variable from inside the nasty generated code that does indeed have parts of it undefined - but also these parts go completely unused and I figured would have been pruned by now / dropped silently? Maybe its not actually the issue :-/

fdxmw commented 1 year ago

I was able to reproduce the "used but not driven" exception with processor_instances_0CLK_a3e7f19d_top.blif (but not top_265f.blif.txt). After adding some more debug logs, it seems the errors are triggered by a lot of wires named manual_registers_r. Examples include:

manual_registers_r[83]
manual_registers_r[52]
manual_registers_r[18]

Searching the .blif file for these wires, they do seem to be undriven:

$ egrep -A1 'manual_registers_r\[(83|52|18)' processor_instances_0CLK_a3e7f19d_top.blif
.names manual_registers_r[18] manual_registers_r[0]
1 1
--
.names manual_registers_r[52] manual_registers_r[34]
1 1
.names manual_registers_r[52] manual_registers_r[35]
1 1
.names manual_registers_r[52] manual_registers_r[51]
1 1
--
.names manual_registers_r[83] manual_registers_r[65]
1 1

Is it possible to change the program that generated this .blif file to omit these wires?

I tried removing them with _remove_unlistened_nets as you suggested, and it passes sanity_check():

$ cat issue422.py
import pyrtl
from pyrtl.passes import _remove_unlistened_nets
from pyrtl.core import working_block
import sys

pyrtl.reset_working_block()
filename=sys.argv[1]
assert len(filename) > 0
print(f"Opening {filename}...", flush=True)
pyrtl.input_from_blif(open(filename))
print("Computing FMAX...", flush=True)

_remove_unlistened_nets(working_block())

timing = pyrtl.TimingAnalysis()
print("Fmax (MHz):", timing.max_freq(), flush=True)

$ python issue422.py processor_instances_0CLK_a3e7f19d_top.blif
Opening processor_instances_0CLK_a3e7f19d_top.blif...
Computing FMAX...
Fmax (MHz): 146.4718590940715
JulianKemmerer commented 1 year ago

Hi there thanks for checking in on this

Yeah these manual_registers_r wires are essentially of the same kind as the read_pipe wires I mentioned above. They are likely indeed partially unused/undriven.

But I would have expected them to not even be rendered in the .blif and would have been optimized away by yosys? As opposed to making PyRTL do the work with _remove_unlistened_nets and more?

The original start of this issue is just about it being slow - it doesnt seem like things are broken at all in PyRTL and I can get by with the _remove_unlistened_nets call for now and accept however slow it is for now - pursuing fixes to the .blif directly.

You folks can close this issue unless it otherwise serves you. Thanks for your attention.