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

naming: Never remove underscores from original name #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gatecat
Copy link
Collaborator

@gatecat gatecat commented Jan 29, 2024

The rules to prevent duplicate or leading/trailing underscores make sense when other characters are being translated to underscores, but in my opinion make less sense when underscores are present in the original cell name.

For example, the gf180 cell gf180mcu_fd_ip_sram__sram512x8m8wm1 would be renamed to gf180mcu_fd_ip_sram_sram512x8m8wm1 by the code as-is, which would cause problems for GDS patching or timing analysis.

If you prefer a more conservative change that just preserves double (or more underscores) in names, I can also take that approach.

@jpc-lip6
Copy link
Collaborator

Yes I noticed this annoying management of underscore. I agree with the principle that we should modify as little as possible the original names. My only concern is about VHDL. We must check that it's still valid for it (if not, move the renaming at this stage).

@gatecat
Copy link
Collaborator Author

gatecat commented Jan 29, 2024

Yes, my bad, I didn't realise that VHDL apparently prohibits two consecutive underscores.

So, if you wanted to use this SRAM with VHDL, you'd have to use an extended identifier in the backend.

@jpc-lip6
Copy link
Collaborator

I confirm, VHDL identifiers does not allow two consecutive underscore, and not trailing one either.

So the translation should be moved at VHDL driver level. If we want to preserve the original names,
we also need to directly load the BLIF file and not go through the VHDL translation in between.

@gatecat
Copy link
Collaborator Author

gatecat commented Jan 29, 2024

I'm a bit concerned about taking names from the BLIF file as-is, because a name might have a . in it (and indeed, this is quite likely when flattening with Yosys) and if that wasn't removed or escaped at the BLIF parse stage, then it would create an ambiguity with a hierarchical name further down the line.

One option for now would be to remove everything but double underscores at the BLIF parse stage and escape those specifically during VHDL export?

@gatecat
Copy link
Collaborator Author

gatecat commented Feb 2, 2024

@jpc-lip6 sorry for the ping, just wondering if you've had a chance to think about my last message here?

@jpc-lip6
Copy link
Collaborator

jpc-lip6 commented Feb 2, 2024

Hello Myrtle, sorry, not yet. Lot of things to do for my lectures last week! Will try as soon as possible.

@jpc-lip6
Copy link
Collaborator

jpc-lip6 commented Feb 5, 2024

Hello Myrtle,

I may have time to look into the problem in the next days. But my first question is the "specification".
If we run straight through Coriolis from BLIF to GDS, we should not have any naming problems.
They should not be affected by . (dot) in names, it may only mislead a human reader looking at
the layout.

On the other hand, the VHDL files generated will not match what has been processed through
Coriolis and may cause discrepencies if we restart and load the VHDL netlist instead of the
BLIF one.

We can also do what you suggest, that is "sanitize" the names coming from BLIF. I fail to find
a description of valid blif identifiers, but is seems they encompass Verilog. So we can add an
option if the BLIF parser to allow the user to request it.

Last question: I'm not sure I understand what you do mean by escaping the double underscores
for the VHDL export.

@gatecat
Copy link
Collaborator Author

gatecat commented Feb 6, 2024

If we run straight through Coriolis from BLIF to GDS, we should not have any naming problems. They should not be affected by . (dot) in names, it may only mislead a human reader looking at the layout.

This is a lot less of a concern for cell names, where it's unlikely anyway, if we get rid of VHDL cleansing in general, you could (and with Yosys flattening, almost certainly will) have a net name with a . in it, say foo.bar.

If you then want to refer to that net, say to create an HTree on it it's ambiguous between a net called foo.bar in the top level or a net called bar in an instance called foo.

Last question: I'm not sure I understand what you do mean by escaping the double underscores for the VHDL export.

Instead of replacing the double underscores with a single one, we could theoretically keep the names intact by using a VHDL extended identifier, so foo__bar becomes \foo__bar\ in VHDL, as this removes the normal rules in place for VHDL names.

@jpc-lip6
Copy link
Collaborator

jpc-lip6 commented Feb 7, 2024

If we run straight through Coriolis from BLIF to GDS, we should not have any naming problems. They should not be affected by . (dot) in names, it may only mislead a human reader looking at the layout.

This is a lot less of a concern for cell names, where it's unlikely anyway, if we get rid of VHDL cleansing in general, you could (and with Yosys flattening, almost certainly will) have a net name with a . in it, say foo.bar.

If you then want to refer to that net, say to create an HTree on it it's ambiguous between a net called foo.bar in the top level or a net called bar in an instance called foo.

In the case of the HTree, the net must be in the core or corona. It cannot be a deep net.
However I see your point, but in which case do we want to refer to a deep net through it's name ?
If need be, what we would build should be a Hurricane::Occurrence which is a pair
of Hurricane::Path and any Entity, in our specific case, a Net. The textual representation
of the Occurrence may be the full path name. I would try, as much as possible on not
relying on naming conventions for complex access. Agreed, there are some places I do it...

Last question: I'm not sure I understand what you do mean by escaping the double underscores for the VHDL export.

Instead of replacing the double underscores with a single one, we could theoretically keep the names intact by using a VHDL extended identifier, so foo__bar becomes \foo__bar\ in VHDL, as this removes the normal rules in place for VHDL names.

Ha OK. I understand and would agree if there wasn't a catch. The point of using VHDL (an, in fact, VST)
is to be backward compatible with Alliance for the purpose of checking. VST (for [V]HDL [st]ructural)
do not support extended identifiers, so I think there is no point in doing it. If I recall correctly, Serge
did develop a Verilog parser driver, we may use it for that.

All in all, I thing this is less of a technical issue than a policy of design flow one. We should clearly
define how the different part of the flow interact, choose (impose) on the users and be public about it.

It is a recurrent problem in design flows, and I think we should have clear guidelines before patching
here and there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants