search

pulp-platform / snitch_cluster

An energy-efficient RISC-V floating-point compute cluster.
https://pulp-platform.github.io/snitch_cluster/
Apache License 2.0
52 stars 54 forks source link

bender: Update to version `0.28.1` #175

Closed fischeti closed 2 months ago

fischeti commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 https://github.com/pulp-platform/bender/issues/184.

colluca commented 2 months ago

I opened a feature request in Bender https://github.com/pulp-platform/bender/issues/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 commented 2 months ago

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 👍