-
Notifications
You must be signed in to change notification settings - Fork 1k
flatten: update scopename regardless of hdlname existance #5519
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
Conversation
|
I'm not completely sure that's necessary? Do we have the exact semantics of these attributes documented somewhere? |
|
The only mention of
Comment in code is yosys/passes/hierarchy/flatten.cc Lines 87 to 88 in e4044e1
|
|
In my opinion, we should not intentionally denormalize attributes such that objects with existing This is for two reasons:
I don't think we ever fail validation due to issues with attributes, so I think (2) is a deal-breaker. But if we ever did denormalize the netlist in this way, we should fail validation in case the scenario from point (2) happens. @KrystalDelusion Do you think you could extend the documentation to accurately reflect the semantics of these attributes? |
|
As an aside, we would actually like to use rope-like things soon for path-like attributes |
|
I wasn't aware of this and it would indeed eliminate (1), but I think (2) on its own is a strong enough argument against duplication, with the verfication caveat above. |
|
If we internalize those attributes, converting them to optimized implementations or constructing them from the start, we can also link hdlname and scopename such that we don't have them desync. They'd just get emitted at the end of a flow but we'd be accessing them within the improved flow with a more meaningful interface than strings. I'm not committing to it, but it seems like an option worth trying. In the mean time, while we rely on attribute-modeled information, I agree there's risks to making it redundant |
|
How do you ingest RTL where |
Added it to the next Docs JF agenda |
|
I added a Discourse thread and will be sharing it around to check if somebody doesn't like their scopenames overridden |
|
|
Noticed this with bram cells, since they usually do have hdlname and in that case cell will only have that attribute set
(* hdlname = "soc cpu cpuregs regs1.0.0" *)But with this change we will also get additional attribute
(* scopename = "soc cpu cpuregs" *)so we can make sure that even cells with public name hdlname will have scopename set.