Altera HLS compatibility - #1529
Conversation
JanFSchulte
left a comment
There was a problem hiding this comment.
Thanks a lot! Looks good to me, with some minor nitpicks.
Please also update the documentation. I think we basically need to replace oneAPI in https://github.com/fastmachinelearning/hls4ml/blob/main/docs/backend/oneapi.rst with a oneAPI/Altera HLS in all cases. Then the status page https://github.com/fastmachinelearning/hls4ml/blob/main/docs/intro/status.rst also needs to be updated to reflect that we support Altera HLS.
|
Thinking about it a bit more, do we need to update the report parsing code to adapt to any changes, or does Altera HLS keep the formats compatible. Also should we add additional tests in the CI for Altera HLS? In many ways those would be redundant with oneAPI, but maybe add a dedicated test just for Altera HLS compilation? |
|
I requested the pytests to run to make sure we don't break anything. For testing with the Altera compiler, I think we need a new docker container. My guess is that we will transition fairly quickly because it seems like oneAPI 2025.0 is hard to get now. |
There was a problem hiding this comment.
Pull request overview
Adds Altera HLS (“ahls”) compatibility to the oneAPI backend by introducing an AHLS compile-time switch and updating SYCL extension namespaces/includes accordingly, while aiming to keep compatibility with existing Intel oneAPI (icpx) flows.
Changes:
- Add
AHLS-gated includes/namespaces for SYCL FPGA extensions, selectors, and experimental properties in oneAPI C++ templates. - Update the oneAPI template CMake to pick
icpxorahlsand defineAHLSwhenahlsis used. - Add an
icpx→ahlsfallback in the Python oneAPI backend build check, and adjust pipe namespace generation in oneAPI type code.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hls4ml/templates/oneapi/myproject_test.cpp | Switch SYCL FPGA extension include and selector namespace based on AHLS. |
| hls4ml/templates/oneapi/myproject_bridge.cpp | Use Altera vs Intel emulator selector depending on AHLS. |
| hls4ml/templates/oneapi/firmware/nnet_utils/nnet_common.h | Switch ac_types include paths based on AHLS. |
| hls4ml/templates/oneapi/firmware/myproject.h | Gate experimental property namespaces (ready_latency, streaming_interface, pipelined) on AHLS. |
| hls4ml/templates/oneapi/firmware/myproject.cpp | Gate task_sequence include and alias on AHLS. |
| hls4ml/templates/oneapi/firmware/defines.h | Switch core SYCL/AC type extension includes based on AHLS. |
| hls4ml/templates/oneapi/CMakeLists.txt | Select icpx or ahls on UNIX and define AHLS for ahls builds. |
| hls4ml/backends/oneapi/oneapi_types.py | Change generated pipe namespace selection logic (currently tied to ahls presence in PATH). |
| hls4ml/backends/oneapi/oneapi_backend.py | Add build-time fallback check for ahls when icpx isn’t found. |
Suppressed comments (1)
hls4ml/backends/oneapi/oneapi_types.py:212
- Same issue as above: selecting
alteravsintelviashutil.which('ahls')in Python can disagree with the actual compiler chosen by CMake, producing C++ that won’t compile in mixed installations. Emit an#ifdef AHLS-guarded namespace in the generated C++ instead.
def declare_cpp(self, indent=''):
compiler_name = 'altera' if shutil.which('ahls') else 'intel'
lines = indent + f'class {self.pipe_id};\n'
lines += indent + (
f'using {self.pipe_name} = sycl::ext::{compiler_name}::experimental::pipe<{self.pipe_id}, '
+ f'{self.type.name}, {self.pragma[-1]}>;\n'
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Direct CMake to use icpx rather than the default C++ compiler/linker on Linux | ||
| # and icx-cl on Windows |
|
The test failures look unrelated. |
|
Thanks, looks good to me now. The remaining question is the test coverage. My understanding is that it would take some effort to add Altera HLS to the setup, so we need to decide if we want to wait for that, or merge this already and add tests later. |
|
Why does it stop short of renaming the backend? The changes make it incompatible with previous releases of oneAPI and the product doesn't seem to be advertised as related to oneAPI on Altera website so it just adds confusion to the user who don't care about technology on which the product is built. |
|
My understanding is that this keeps compatibility with oneAPI 2025.0? So I wouldn't drop oneAPI from the docs, but we could change it to describe AHLS independent of oneAPI, independent of how they are basically the same in the backend. |
|
It's a bit confusing. If I want to use it, what should I put in |
|
I overlooked the part with the conversion. I agree that it should be separate there as well so that you pass |
|
Regarding environments, it's not for us to solve, but we're creating a problem. Only because of the way the check is implemented is this an issue. Far simpler solution would be to have |
|
We don't want a very light AlteraBackend. If anything, it's the oneAPI backend that should be light. Otherwise, we are stuck with what we have now with the Vitis/Vivado situation. It doesn't seem like you can even install oneAPI 2025.0 any more. |
|
There was a suggestion from the Altera folks to rename the backend, by the way. I think it was not included in this to be less disruptive. |
|
Agree with that. So it's the Altera one that is the main, and oneAPI is the light wrapper that gets removed eventually. |
|
Do you think it's worth the effort to make a whole new backend just for a compiler name change plus some include paths and namespaces? |
| * Intel HLS | ||
| * Vitis HLS | ||
| * Catapult HLS | ||
| * Altera HLS (experimental) |
There was a problem hiding this comment.
Doesn't Altera HLS refer to the old Intel HLS, currently deprecated. It seems like the name is Altera HLS IP Gen, right? There is discussion on whether it should be a separate backend from oneAPI or if these should be one, as the code has it now. The documentation should follow the code, whatever we decide. I would also remove the experimental, and add (deprecated) to Intel HLS.
|
I don't think it is a whole new backend. It sounds large but is three simple stages (on top of what's already done):
Everything should work as before, including any tests. When the time comes, it's just a matter of deleting the oneAPI backend class and we can remove the |
|
I am fine with that proposal. I think AI can pretty quickly change the file names and class types for part 1, like |
|
This is replaced by #1533 |
|
Reopening just to run pytests. (DO NOT MERGE.) |
Description
Type of change
Tests
Test Configuration:
N/A
Checklist
pre-commiton the files I edited or added.