Parallel conv partial fix #1380

calad0i · 2025-09-08T18:25:35Z

Description

Provide partial fix to the parallel conv issue on Vitis:

Pointwise behavior is fixed. However, as I still can't recover the correct II behavior by tuning the pragmas back and forth, it is disabled except global n_partitions==1 case.
- rf and pf isolated
- rf, precision, trace flags are inherited now
- use template instead of codegen for mostly static code
Standard conv:
- pf give proper II now
- Pipelining appears to work (2025.1 and 2023.2, but only tested on minimal examples now)

Each layer still fully blocks, and the dataflow pragma is not having the ideal behavior (i.e., kernel level piplining). At least, we have correct global II now and resource indeed decreases with lower pf. Vitis 2025.1 hangs at cosim but 2023.2 was fine, guess it is a bug on the vitis side. Didn't test with more version.

Since synth test CI is still not in place, I can't fire up a full regression test now. Please check if this PR breaks your use case.

Type of change

Other (performance tuning)

Tests

When will synthesis tests be ready?

Checklist

all

…ll be too high)

jmitrevs · 2025-09-08T20:30:18Z

Considering "rf, precision, trace flags are inherited now", doesn't this now give you warnings of them being overwritten? The reason they were not copied is because they were recreated again from the config, so if you copy them, they get overwritten when the config is parsed (and you get the warning). And you do have to recreate the config in order to make sure precisions are correctly transferred to the new pointwise, since they get generated again. Truthfully this behavior of hls4ml is a bit weird and should maybe be revisited, but just copying those values doesn't fix it.

jmitrevs · 2025-09-08T20:33:03Z

hls4ml/backends/catapult/passes/pointwise.py

    def transform(self, model, node):
        dim = node.__class__.__name__[-2:]  # '1D' or '2D'
-        new_attrs = {k: v for k, v in node.attributes.items() if k not in ('trace', 'precision', 'reuse_factor')}
+        new_attrs = node.attributes.attributes.copy()


I mentioned this in a comment in the conversation. I think trace, precision, and reuse_factor are regenerated no matter what, so the values you copy here get overriden (unless something has changed from before). It may be an hls4ml behavior worth revisiting and potentially revising, but I don't think this change fixes anything.

reuse_factor defined under Model key is not propagated as expected otherwise. Rm'ed warning if the update opr is trivial

Did you see why it's not propagated properly? Shouldn't it come from a configuration in the first place?

jmitrevs · 2025-09-08T20:40:40Z

hls4ml/backends/vivado/passes/convolution_templates.py

+                // mult_params['n_partitions']
            )
-            mult_params['n_out'] = int(node.get_attr('in_width') * node.get_attr('n_filt') / mult_params['reuse'])
+            mult_params['n_out'] = node.get_attr('in_width') * node.get_attr('n_filt') // mult_params['n_partitions']


Are n_partitions and mult_params['n_partions'] different?

jmitrevs · 2025-09-08T20:51:45Z

hls4ml/templates/vitis/nnet_utils/nnet_conv1d.h

    // Inlining helps reduce latency, but may also cause timing issues in some cases, use carefully.
-    //#pragma HLS INLINE recursive
+    // But without inlining Vitis HLS doesn't respect the parallelization factor config ¯\_(ツ)_/¯
+    // #pragma HLS PIPELINE II = CONFIG_T::reuse_factor * CONFIG_T::n_partitions


Maybe erase commented out pragmas throughout?

jmitrevs · 2025-09-08T20:53:21Z

hls4ml/templates/vitis/nnet_utils/nnet_conv1d.h

 };

+template <class data_T, class res_T, typename CONFIG_T>
+class BatchedDenseForConv1D : public nnet::Conv1DKernel<data_T, res_T, CONFIG_T> {


Can we add a comment to say the purpose of this code (and also for the 1D version)?

The PR allows modification from contributors. Feel free to add some if you find it necessary.

vloncar · 2025-09-23T14:29:21Z

hls4ml/templates/vitis/nnet_utils/nnet_conv1d.h

                     typename CONFIG_T::weight_t weights[CONFIG_T::filt_width * CONFIG_T::n_chan * CONFIG_T::n_filt],
                     typename CONFIG_T::bias_t biases[CONFIG_T::n_filt]) {
-        //#pragma HLS INLINE region
+        // #pragma HLS INLINE recursive


This is already disabled, and now remains disabled but with a different action. Why?

Was enbaled and disabled when changing things around. Though, since inline region doesn't exist in vitis, the current one seems to be making a bit more sense...

vloncar · 2025-09-23T14:34:11Z

hls4ml/backends/vivado/passes/convolution_templates.py

-                node.get_attr('in_width') * node.get_attr('n_chan') * node.get_attr('filt_width') / mult_params['reuse']
+        if is_pointwise_parallel_latency and n_partitions == 1:
+            mult_params['n_in'] = (
+                node.get_attr('in_width') * node.get_attr('n_chan') * node.get_attr('filt_width') // n_partitions


So basically, this change now forces the parallelization to be based on n_partitions (in a sense it is PF) and not reuse_factor. I agree it is more intuitive, do we have to follow up in any docs?

The current behavior of replacing pf by rf is an undocumented change. This only aligns it back to the standard convolution behavior (albeit still not fully working). With the last pragma change maybe it is fixed, but will need to check again

vloncar · 2025-09-23T14:36:45Z

hls4ml/backends/vivado/passes/convolution_templates.py

-        if is_pointwise_parallel_latency:
-            mult_params['n_in'] = int(
-                node.get_attr('in_width') * node.get_attr('n_chan') * node.get_attr('filt_width') / mult_params['reuse']
+        if is_pointwise_parallel_latency and n_partitions == 1:


Doesn't this restrict the whole function to a single (fully parallel) case?

Also, ff we ever fix the HLS code to make it work for any combination of PF/RF, will this line be the only change we need to make on the python side? If so, maybe document it

This does restrict it to the full parallel case, since otherwise the performance was worse than the standard implementation. Will need to check again with the last pragma change...

I thought that operation instead of function would actually cause the synthesis to fail, not the pragma being ignored

In vivado_hls it fails, but in vitis it just ignored the pragma

calad0i added 3 commits September 8, 2025 10:10

pointwise conv 1d partial fix

f7535b7

pointwise inherit all property

e88b856

inline for parallel conv, pointwise only for npart=1 (otherwise II wi…

7a0ee88

…ll be too high)

JanFSchulte added this to the v1.2.0 milestone Sep 8, 2025

avoid unexpected shared state

d077b72

calad0i added the please test Trigger testing by creating local PR branch label Sep 8, 2025

calad0i requested review from JanFSchulte, jmduarte, jmitrevs and bo3z September 8, 2025 18:43

jmitrevs reviewed Sep 8, 2025

View reviewed changes

rm useless warning

f349e41

JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 9, 2025

calad0i added 2 commits September 9, 2025 06:05

rm line

39d416b

.

7a6f1db

jmitrevs removed the please test Trigger testing by creating local PR branch label Sep 22, 2025

Merge branch 'main' into conv-partial-fix

343ae5c

jmitrevs added the please test Trigger testing by creating local PR branch label Sep 22, 2025

fix pragma

e530e18

calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 23, 2025

vloncar reviewed Sep 23, 2025

View reviewed changes

Parallel conv partial fix #1380

Are you sure you want to change the base?

Parallel conv partial fix #1380

Uh oh!

Conversation

calad0i commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Tests

Checklist

Uh oh!

jmitrevs commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmitrevs Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmitrevs Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calad0i commented Sep 8, 2025 •

edited

Loading

jmitrevs commented Sep 8, 2025 •

edited

Loading

jmitrevs Sep 8, 2025 •

edited

Loading

jmitrevs Sep 8, 2025 •

edited

Loading