hls4ml icon indicating copy to clipboard operation
hls4ml copied to clipboard

Remove #INLINE pragma to fix SeparableConv1D with Vitis backed

Open qberthet opened this issue 1 year ago • 4 comments

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

  • [x] 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

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

qberthet avatar Oct 25 '23 08:10 qberthet

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)

vloncar avatar Oct 25 '23 09:10 vloncar

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.

qberthet avatar Oct 25 '23 10:10 qberthet

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 :-)

vloncar avatar Oct 25 '23 15:10 vloncar

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..

qberthet avatar Oct 25 '23 17:10 qberthet