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

Expose src attribute to yosys #35

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

Conversation

pepijndevos
Copy link
Contributor

@pepijndevos pepijndevos commented Aug 20, 2019

Currently the source location data present in GHDL is not preserved when creating a Yosys netlist.
This PR adds the /src attribute to every cell when present in GHDL.

Note that this does not yet fix the problem where SymbiYosys just displays the instance name, but it's a good change nonetheless. I'll do a bit more digging on the SymbiYosys problem.

This requires PR ghdl/ghdl#898 on GHDL for the Get_Source function.

@pepijndevos
Copy link
Contributor Author

pepijndevos commented Aug 20, 2019

Oh, it might be that it needs a dollar prefixed name
Otherwise it thinks it is a labeled assertion (label: assert(x);) and therefore prints the name instead of the location

There appears to be some logic for this in ghdl.cc regarding Sname_Artificial, so maybe there is a bug there. But I'm stuck on magic numbers again. The prefix of my assume gate is a magnificent 378. Ah... ok... I see...

@tgingold
Copy link
Member

Do we need to set the location on all gates, or just on assume/assert ?

@pepijndevos
Copy link
Contributor Author

I mean, in principle there is no reason an AND gate can't have a corresponding source location. It's up to the GHDL side which ones are preserved.

@pepijndevos
Copy link
Contributor Author

I'm still completely stuck on why assert/assume are not system names in to_str, so I hope you have an idea what's going on here. Once that's fixed, source locations in SymbiYosys should be working.

@pepijndevos
Copy link
Contributor Author

What's the status of this and ghdl/ghdl#898 ?
My understanding is that not a lot of GHDL cells have a location set, but I think it could be merged and extended later with more locations?

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