search

EPFL-LAP / dynamatic

DHLS (Dynamic High-Level Synthesis) compiler based on MLIR
Other
49 stars 14 forks source link

New flag `skip_polygeist` in build.sh script #84

Closed Carmine50 closed 2 months ago

Carmine50 commented 2 months ago

Added a new flag in the build.sh script to skip POLYGEIST/LLVM compilation specifying an existing folder with polygeist compiled

Jiahui17 commented 2 months ago

I think one small problem here is that the verifier toolchain doesn't know where to find Polygeist (https://github.com/EPFL-LAP/dynamatic/blob/f72137a9632f6b165a76ffe3e857f61961a5bf6a/tools/dynamatic/scripts/compile.sh#L18).

Jiahui17 commented 2 months ago

Thanks for the contribution! Left a few minor comments.

I think one small problem here is that the verifier toolchain doesn't know where to find Polygeist

Do you mean that ninja check-dynamatic won't work out of the box when building with an external version of Polygeist? If so I may be fine with it given that, to me, this is not the regular way of building Dynamatic, and "regular users" will just ignore it. echo-ing a warning about this when building with the --skip-polygeist flag may be good in that case.

No I meant that when Polygeist compiles C code to mlir, it needs to know where to find the headers

https://github.com/EPFL-LAP/dynamatic/blob/f72137a9632f6b165a76ffe3e857f61961a5bf6a/tools/dynamatic/scripts/compile.sh#L77

Carmine50 commented 2 months ago

@lucas-rami I resolved the previous comments. Thank you!

Carmine50 commented 2 months ago

@Jiahui17 Maybe a solution would be a common environment variable that specifies the polygeist path? If present it would use this path, if not it uses the default path. This environment variable could be automatically added in the .bashrc file once dynamatic is compiled with the skip-polygeist flag

Jiahui17 commented 2 months ago

Maybe we can let the user manually simlink polygeist in the project root, and just tell the build script to skip building it (not sure if it is cleaner and less manual work than putting a new environment variable in bashrc

lucas-rami commented 2 months ago

Maybe a solution would be a common environment variable that specifies the polygeist path? If present it would use this path, if not it uses the default path. This environment variable could be automatically added in the .bashrc file once dynamatic is compiled with the skip-polygeist flag

I would prefer a solution that doesn't require the user to manually edit their environment variables, as I always find this cumbersome when I install software on any machine. I am also not sure that modifying a user's .bashrc file automatically is good practice (this filer may not even exist if they use a different shell).

Maybe we can let the user manually simlink polygeist in the project root, and just tell the build script to skip building it (not sure if it is cleaner and less manual work than putting a new environment variable in bashrc

I prefer this but I would like to avoid making users do manual work in their (file) system to make this work, especially since it is really a frontend problem.

I think one other option is to create a new frontend command in the same spirit as set-dynamatic-path but for Polygeist. By default, the Polygeist path would be the "normal one" (again similarly as for the Dynamatic path, which is the current working directory by default), but if one inputs the set-polygeist-path /path/to/polygeist command before the compile command then the compile script would know to go look for the headers elsewhere.

In any case, I will merge this as is, feel free to make another PR to resolve this issue.