$scopeinfo support

[jix]

This PR adds $scopeinfo cells to preserve information about the hierarchy during flattening, in particular attributes including src.

This is still missing a kernel header containing utilities for easily querying $scopeinfo data.

[povik]

Nit: typos here, either "attribute is handled" or "attributes are handled", "separated"

[povik]

I would suggest not referring to how this was handled in the past, just explain source locations would be collected from different points on the hierarchy and combined without distinction on the cell post flattening.

[nakengelhardt]

(Also FYI strpool uses | not whitespace as separator).

[jix]

Right, I must have been thinking of hdlname, I'll update the description.

[povik]

You have probably thought this out already and I am curious to hear it. Should this be cell or module?

[povik]

(Also sorry for bikeshedding but the only other occurences of enums on cell parameters within Yosys I have seen were on some vendor cells, where the values were fully uppercase. Also e.g. in select syntax we would have the parameter name and its value next to each other. For reasons of consistence we could say the values for an enum-style parameter should be uppercase.)

[whitequark]

You have probably thought this out already and I am curious to hear it. Should this be cell or module?

It refers to a module instantiation (a module was written by the designer at some point) so I would say "module" and not "cell".

(Also sorry for bikeshedding but the only other occurences of enums on cell parameters within Yosys I have seen were on some vendor cells, where the values were fully uppercase. Also e.g. in select syntax we would have the parameter name and its value next to each other. For reasons of consistence we could say the values for an enum-style parameter should be uppercase.)

I think it's consistent enough with lowercase. In Verilog and RTLIL both the module is spelled as module, and when extending this I think we might have other scopes such as begin or maybe struct. We aren't writing ALGOL to have BEGIN :)

[povik]

I think it's consistent enough with lowercase. In Verilog and RTLIL both the module is spelled as module, and when extending this I think we might have other scopes such as begin or maybe struct. We aren't writing ALGOL to have BEGIN :)

No contest

[jix]

I couldn't decide between "cell" and "module" as which one is more appropriate depends on whether you want to indicate what caused the presence of the scope i.e. the entity that has the same hierarchical name as the $scopeinfo cell, in which case it would be "cell" (or "instance" if we're using verilog not RTLIL terminology). If we're indicating what defines the contents of the scope then "module" is clearly the correct choice.

Since both approaches make sense to me and changing it is trivial at this point, I went with an arbitrary choice.

Given the feedback so far, I'd change this to "module".

[whitequark]

I am not opposed to instance, though I feel it might be a little too ambiguous.

[povik]

Looks good to me

[nakengelhardt]

Currently opt_merge considers all $scopeinfo cells identical and will remove all but one of them. Should we special-case $scopeinfo there too, or should we perhaps have a global ignorelist in yosys_celltypes that all optimization passes can check against so as to not touch those cells?

[whitequark]

or should we perhaps have a global ignorelist in yosys_celltypes that all optimization passes can check against so as to not touch those cells?

I do want opt_merge to merge the $debugprobe cells (using a special treatment) so this would not benefit that effort.

[povik]

Currently opt_merge considers all $scopeinfo cells identical and will remove all but one of them. Should we special-case $scopeinfo there too, or should we perhaps have a global ignorelist in yosys_celltypes that all optimization passes can check against so as to not touch those cells?

Huh, I thought there was some logic to consider cells equal only if the attributes match (except for src I suppose). Or was that only for wires?

…

Message ID: ***@***.***>

[jix]

or should we perhaps have a global ignorelist in yosys_celltypes that all optimization passes can check against so as to not touch those cells?

I think that makes sense if we think that list will grow or can already start out containing other cells too. If it's only going to be $scopeinfo I'd say we add code specific for that and can move to an ignore list later when there are more cells that should be treated the same way.

[jix]

I'm currently working on getting this PR ready (as wall as a followup PR with code to build a $scopeinfo aware hierarchical index for use in yosys passes) and I'm not sure how the various backend passes should treat $scopeinfo. In particular backend passes that are used to transfer synthesis results to other tools.

I'm assuming that the current state of this PR is likely to break existing synthesis setups with other tools potentially failing on unexpected $scopeinfo cells. I can also imagine the $scopeinfo data could eventually be of use for some of the tools that would currently fail due to its presence.

Since I use yosys almost exclusively for FV, I could use some input on how to handle this, though.

[whitequark]

Personally, I would be OK with every backend except for CXXRTL and JSON ignoring $scopeinfo entirely, since it is a Yosys internal cell that will definitely break such other tools. I don't know, however, how to deal with people who do want e.g. $scopeinfo in their structural Verilog output.

Maybe we could handle that when we encounter it?

[jix]

That also makes sense to me, but for JSON there is the issue that on one hand the JSON backend is intended to include all of the RTLIL netlist but also that nextpnr which reads that JSON output will fail with ERROR: Unable to place cell 'soc.cpu', no BELs remaining to implement cell type '$scopeinfo' with this PR.

A way to work around this would be to have the corresponding synth passes explicitly delete $scopeinfo cells when using the -json option until nextpnr at least knows to ignore those cells. This would still break synth_…; write_json output.json when used with nextpnr, but in that case you can manually insert a delete */t:$scopeinfo between synth_… and write_json and it seems like all the nextpnr documentation and examples use -json anyway.

Does that sound acceptable?

Alternatives would be to get nextpnr to ignore these cells before merging this PR or to ignore $scopeinfo by default also for the JSON backend.

[whitequark]

Alternatives would be to get nextpnr to ignore these cells before merging this PR

I think we can do this.

[gatecat]

I am happy to add this if we really want to avoid removing these cells in Yosys - however, despite the lack of any firm compatibility guarantees between Yosys and nextpnr, I do try hard to avoid situations like this (not least because they make bisecting more annoying) and it's been some time since we last had to introduce one.

[whitequark]

I think we can just not emit the $scopeinfo cells anywhere but in CXXRTL and RTLIL then.

[povik]

How about both deleting the $scopeinfo in the synth_ passes ahead of JSON export, and also teaching nextpnr to ignore those? That would address the potential incompatibility (at least for the usual way of invoking the flow), while keeping the JSON backend on par with RTLIL.

[povik]

Now that I think of it, we want $scopeinfo as part of the JSON output for more informed logging on part of nextpnr! E.g. when explaining the critical path.

[povik]

Well, at least in the future, when the synthesis flow will stop losing source attribution because of a roundtrip through ABC...

[whitequark]

Now that I think of it, we want $scopeinfo as part of the JSON output for more informed logging on part of nextpnr! E.g. when explaining the critical path.

Yes, which is why I proposed to have it in JSON initially.

[whitequark]

We will have the exact same problem with $probe cells, so the solution for $scopeinfo should be extensible to that cell at least.

Similarly with any $check cells that end up in a synthesis flow. (It might be meaningful to execute those in hardware--eventually!)

[povik]

We will have the exact same problem with $probe cells, so the solution for $scopeinfo should be extensible to that cell at least.

Similarly with any $check cells that end up in a synthesis flow. (It might be meaningful to synthesize those--eventually!)

As I see it, $probe and $check can simply be stripped at some point (as would $print), this is brought up in #4122, but $scopeinfo we want to get to the P&R tool for logging.

[whitequark]

Right, even if I do want $print in hardware I don't want nextpnr to have the logic for it.

[gatecat]

Either way, I'll update nextpnr to ignore $scopeinfo because that is never going to hurt (and in the future, extend the frontend to populate nextpnr's own hierarchy structures with the info from it, although these haven't been touched for a while and might need some changes too).

[jix]

I think $probe and $scopeinfo are different from $check in that they would start appearing without any changes in the input source, so failing when they are present breaks existing flows with existing inputs whereas when anything that doesn't already support $assert fails on $check that would not be a regression. (And all the supported existing FV use cases would automatically support $check when clk2fflogic and async2sync do.) For this PR I'm more concerned about regressions, and think improvements on how $assert or $check are handled can be done separately.

My current plan for $scopeinfo would be: I add checks to ignore it to all backends besides RTLIL, CXXRTL and JSON. For now I'd explicitly check for $scopeinfo, but it will be easy to extend it with $probe or to eventually move it to some better cell-classification infrastructure should that appear as part of #4122 or otherwise. IMO the important part for now is identifying all the places where such checks are needed. Changing the exact check used later on when required for new cells or to support new use cases is not a big deal, IMO.

I don't have any opinion, though, on whether to add something temporary and/or optional to the JSON backend and/or to the relevant synth passes to ignore $scopeinfo for now, to keep compatibility with older nextpnr versions. I also don't think I'm in the best position to make a decision on that, but I can implement whatever consensus there is as part of this PR. (But I'm not sure there is one at this point?)

[gatecat]

My suggestion is:

• in the short term, we strip $scopeinfo from the JSON
• in a couple of months, once there's been a nextpnr release that's at least able to ignore it, and support in nextpnr that actually makes use of it, we can consider to start outputting it.

[whitequark]

My suggestion is:

• in the short term, we strip $scopeinfo from the JSON
• in a couple of months, once there's been a nextpnr release that's at least able to ignore it, and support in nextpnr that actually makes use of it, we can consider to start outputting it.

SGTM

[whitequark]

I've been looking into integrating this with CXXRTL. I've actually hit a small amount of trouble since I'm not entirely sure how to expose both module and cell attributes, but this is CXXRTL-side. I will probably be done with it by the end of next week, but I do not mind having this PR merged before then.