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

Remove #INLINE pragma to fix SeparableConv1D with Vitis backed #898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qberthet
Copy link
Contributor

Fixes #897

Description

With Vitis backend, when #pragma HLS function_instantiate is used, #pragma HLS INLINE can only be used with the off option to forbid inlining. Using only #pragma HLS INLINE results in the error mentionned in #897.

From what I understand from https://docs.xilinx.com/r/2022.2-English/ug1399-vitis-hls/Function-Instantiation, removing this pragma does not means that inlining will not be performed when possible.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Tests

As this is tested during CSynth, no PyTest are provided, but the test script in #897 can be used to test that the problem is indeed fixed.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.

@vloncar
Copy link
Contributor

vloncar commented Oct 25, 2023

This won't get inlined automatically by the compiler, meaning +1 on latency of the inner product for every output pixel due to the extra function call. It's the function_instantiate pragma that we should remove. That pragma is a leftover from the wild '10s of Vivado HLS where tricks like that were actual optimizations (note that there will only ever be one instance of this function since the weights array is fixed, so it is useless)

@qberthet
Copy link
Contributor Author

If function_instantiate is removed, there is then a clash with #pragma HLS PIPELINE .... If this one is removed, the issue is solved, but again, not sure about the impact on resulting rtl.

@vloncar
Copy link
Contributor

vloncar commented Oct 25, 2023

Looking at the surrounding code there's already a inline recursive pragma before each call to this function. Does that get ignored? Is inlining itself a problem (that messes with the pipeline of the depthwise_conv_1d_buffer_cl that ultimately calls this) or is the issue the existence of additional pipeline directive in the depthwise_product (which should be ignored with a warning, but is promoted to an error in Vitis)? I think we should test if this works better inlined or not, and if not, prevent inlining by removing those pragmas (and perhaps even keeping the one in depthwise_product but with off). Just not with function_instantiate :-)

@qberthet
Copy link
Contributor Author

To be honest, I have no clue about all this. I just have the impression from the error message that the multiple pragma are simple mutually exclusive with Vitis (obviously no issue with Vivado).

I am willing to dig a bit in all this and do some tests, but not sure when I will have the time to do it..

@jmitrevs jmitrevs added this to the v1.0.0 milestone Dec 15, 2023
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.

SeparableConv1D fail CSynth with Vitis backend
3 participants