Skip to content

[Deps] Update finn-hlslib #29

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

Merged
merged 2 commits into from
Jan 31, 2025
Merged

[Deps] Update finn-hlslib #29

merged 2 commits into from
Jan 31, 2025

Conversation

iksnagreb
Copy link

After switching to the recent finn-hlslib version (dev branch), the ap_float header is included by default whenever you are using some operation which requires the flatten function (e.g. attention, elementwise-binary). However, ap_float requires Vitis HLS 2024.2 while we are still stuck on 2022.2... For now this is a workaround, I'd like to properly fix this via some feature test macros conditionally disabling compilation of the float section...

@fpjentzsch
Copy link

Does this (Xilinx/finn-hlslib#154) mean we can make this point to Xilinx/finn-hlslib instead of your fork?

@iksnagreb
Copy link
Author

I will try this later, probably yes.

@iksnagreb
Copy link
Author

Now pointing back to Xilinx/finn-hlslib and got the radioml-transformer build passing so we do not need the fork for this. Could be merged now.

We need to remember to add the necessary includes to the code generation though, as soon as we want to make use of the ap_float (and also ap_fixed) support now. But that's not a dependency issue, rather a code generation issue for affected hardware operators.

@iksnagreb iksnagreb marked this pull request as ready for review January 31, 2025 17:48
@fpjentzsch fpjentzsch changed the title [Deps] Switch to custom finn-hlslib without ap_float includes [Deps] Update finn-hlslib Jan 31, 2025
@fpjentzsch fpjentzsch merged commit 2601b60 into dev Jan 31, 2025
1 check passed
@fpjentzsch
Copy link

@iksnagreb in the meantime other things in hlslib changed, at least one of which breaks things (the 8 failing tests):
The UpsampleNearestNeighbour_Batch was removed (Xilinx/finn-hlslib@2b9f065) in favor of the new Upsampler, which we do net yet integrate in FINN. Can we pull this integration in from somewhere or do we need to roll back to a custom hlslib fork again?

@iksnagreb
Copy link
Author

Hm, there is Xilinx#1202 (this does not really seem ready to be used though...) or maybe some solution Lucas has been working on to make this usable for one of the PG models? Otherwise we could fix this by changing the code generation to generate code for the legacy interface, which actually should not have been removed but just renamed, so we just need to rename it during code generation as well...

@fpjentzsch
Copy link

Fixed in #40

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