yosys_import: Support JSON produced with -compat-int
#2847
Conversation
RyanGlScott
added
the
subsystem: hardware
podhrmic
left a comment
podhrmic
left a comment
There was a problem hiding this comment.
LGTM, although I haven't actually ran it.
| Just (Aeson.Number n) -> n > 0 | ||
| Just (Aeson.String t) -> textBinNat t > 0 | ||
| Just v -> | ||
| panic "cellToTerm" |
There was a problem hiding this comment.
This error reports an ill-formed input file and shouldn't be a panic...
There was a problem hiding this comment.
Hm, perhaps so. At the same time, this is already an established convention in this module (see existing uses of panic in the connWidthNat and input functions further down), so if we change how errors are reported here, we should also change how they're reported elsewhere.
What would be your preference for how to report these errors?
There was a problem hiding this comment.
There's a YosysError datatype in SAWCentral.Yosys.Utils that has an Exception instance, and some other parts of our Yosys code throw these as exceptions. For consistency it might be nice to do the same, perhaps adding a new constructor to the YosysError type.
Except definitely don't use throw like the existing code does; all such uses need to be changed to throwIO.
These local functions like connSigned with pure types should be changed to monadic types if they are partial.
There was a problem hiding this comment.
My thought would be just to add -- XXX this should not be a panic and I'll fix it when I come through with the new error infrastructure, which hopefully won't be that much longer.
I would rather not introduce any new explicit throwing, even IO-level throwing, because there's an impenetrable mess of unsystematic throw and catch all over everywhere that I don't think can be cleaned up, only removed. This will become clearer as other things move forward...
There was a problem hiding this comment.
In that case, I've settled on leaving XXX-style comments next to each of the panics in this function. See fe4c3e4.
There was a problem hiding this comment.
Leaving it for later is fine with me.
RyanGlScott
force-pushed
the
rgs/yosys-cell-parameters-ints
branch
from
4065fd6 to
f3d43dd
Compare
|
I just merged #2832, which does a bunch of extra refactoring and reformatting of Yosys code. I realized it will probably cause a bunch of merge conflicts for this PR; sorry about that! |
To do so, we parse cell parameter values more generally to accept all possible JSON values, and then we refine the code which parses `*_SIGNED` parameters to accept both strings (which is what Yosys's `write_json` command normally produces for parameters) and numbers (which is what is produced when using `write_json -compat-int`). Fixes #2846.
…ldn't be panicking
RyanGlScott
force-pushed
the
rgs/yosys-cell-parameters-ints
branch
from
f3d43dd to
fe4c3e4
Compare
|
I've rebased on top of #2832. Surprisingly, there were no merge conflicts! |
To do so, we parse cell parameter values more generally to accept all possible JSON values, and then we refine the code which parses
*_SIGNEDparameters to accept both strings (which is what Yosys'swrite_jsoncommand normally produces for parameters) and numbers (which is what is produced when usingwrite_json -compat-int).Fixes #2846.