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

bender: Update to version 0.28.1 #175

Merged
merged 3 commits into from
Sep 28, 2024
Merged

bender: Update to version 0.28.1 #175

merged 3 commits into from
Sep 28, 2024

Conversation

fischeti
Copy link
Contributor

@fischeti fischeti commented Aug 8, 2024

Updates the bender version in the docker container and in the iis-setup.sh script to the newest version.

The flist bender command slightly changed, which now prints only the source files without the +incdir+ paths. The new flist-plus command also prints the +incdir+'s and additionally +define+'s, which have to be filtered now.

(Alternatively, we could also use the flist command and remove the sed if we don't need the header files as make prerequisite, but I decided to keep the functionality the same)

@fischeti fischeti changed the title misc: Update bender to 0.28.1 bender: Update to version 0.28.1 Aug 8, 2024
@fischeti fischeti marked this pull request as ready for review August 8, 2024 18:41
Copy link
Collaborator

@colluca colluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the flist command only lists source files and not included headers? Cause then there's not really a point in upgrading Bender (not for this feature at least, since we're not using the new flist command anyways).

Maybe we can suggest a feature request for Bender to implement an flist command which actually lists all prerequisites.
It could perhaps separate commands for flist-sources and flist-headers, if anyone needs them to compose arbitrary tool commands. But otherwise just including the headers in the flist command sounds more reasonable than what it currently implements.

Also, @fischeti do you understand why we append /*/* with sed at the end of the included directories? I would expect /* to suffice, since the tool anyways only looks at the files directly under +incdir+, if I am not mistaken.

@fischeti
Copy link
Contributor Author

fischeti commented Aug 9, 2024

So the flist command only lists source files and not included headers? Cause then there's not really a point in upgrading Bender (not for this feature at least, since we're not using the new flist command anyways).

It is not really critical to update right now, but since the default version at IIS machines is the newest one if doesn't hurt to upgrade (eventually, we will have to do it anyway).

Maybe we can suggest a feature request for Bender to implement an flist command which actually lists all prerequisites. It could perhaps separate commands for flist-sources and flist-headers, if anyone needs them to compose arbitrary tool commands. But otherwise just including the headers in the flist command sounds more reasonable than what it currently implements.

I believe the flist is kind of a standard in EDA tools, so I am not sure if this can/should be changed. There are flags (e.g. --only-headers), but they only work for Vivado at the moment. Maybe the could be supported as well for flist and/or flist-plus

Also, @fischeti do you understand why we append /*/* with sed at the end of the included directories? I would expect /* to suffice, since the tool anyways only looks at the files directly under +incdir+, if I am not mistaken.

Yes, the header files are nested usually as include/protocol/typedef.svh, because if you have multiple protocols and typedef.svh files you can them include them like this:

`include "protocol1/typedef.svh"
`include "protocol2/typedef.svh"

@colluca
Copy link
Collaborator

colluca commented Aug 9, 2024

I believe the flist is kind of a standard in EDA tools, so I am not sure if this can/should be changed. There are flags (e.g. --only-headers), but they only work for Vivado at the moment. Maybe the could be supported as well for flist and/or flist-plus

Yeah you're right, a dedicated command would be better indeed.

Yes, the header files are nested usually as include/protocol/typedef.svh, because if you have multiple protocols and typedef.svh files you can them include them like this:

`include "protocol1/typedef.svh"
`include "protocol2/typedef.svh"

I see, that makes sense, but this sed solution is still extremely fragile. If any dependency does it differently, i.e. doesn't have a nested protocol folder, we would get No rule to make target 'incdir/*/*' errors. Correct me if I'm wrong.

If we don't have a reliable way to list all header prerequisites I would rather drop them altogether, apart from the known ones in our own repo, which are anyways the only likely ones to be edited (as far as one doesn't bender clone).

Maybe Verilator has an option to list all header prerequisites, this gist also mentions how to achieve this with Verilog-perl https://gist.github.com/brabect1/c1c377321cdb266d2d46. But indeed this would be too much effort to implement in Bender, unless we take a different approach, e.g. to specify exactly which header files a package exports, in addition to the include directories.

Edit

I checked and Verilator doesn't seem to have an option to do this.

I spoke with @micprog and he suggests another simple alternative to support this in Bender, namely listing all header files recursively found under the incdirs. This would probably not work, see pulp-platform/bender#184.

@fischeti fischeti marked this pull request as draft September 4, 2024 19:04
@colluca
Copy link
Collaborator

colluca commented Sep 26, 2024

I opened a feature request in Bender pulp-platform/bender#184.

I believe we can go on with this PR as is, and perhaps the aspect mentioned above can later be improved in a separate PR.

@fischeti
Copy link
Contributor Author

I opened a feature request in Bender pulp-platform/bender#184.

I believe we can go on with this PR as is, and perhaps the aspect mentioned above can later be improved in a separate PR.

Sounds good 👍

@colluca colluca marked this pull request as ready for review September 28, 2024 10:52
@colluca colluca requested a review from viv-eth as a code owner September 28, 2024 10:52
@colluca colluca merged commit c95a0d8 into main Sep 28, 2024
27 checks passed
@colluca colluca deleted the update-bender branch September 28, 2024 10:53
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