Skip to content

Conversation

@SpiritSeeker
Copy link

  • Infer thresholding HW layer only when the input and thresholds are both integers or both FP32.
  • Enable use of FPARG in thresholding RTL for FP32 comparisons
  • Add tests for FP32 thresholding

@auphelia
Copy link
Collaborator

Thank you @SpiritSeeker ! Looks great :)

@auphelia auphelia merged commit 95a46d3 into Xilinx:dev Sep 16, 2025
2 checks passed
@SpiritSeeker SpiritSeeker deleted the fp-thresholding branch September 16, 2025 15:51
@fpjentzsch
Copy link
Collaborator

@SpiritSeeker @auphelia On our end this broke 3 tests. Please double check and see if you need these fixes on your side as well.

1) test_convert_to_hw_layers_cnv_w1a1[False]

The input quantizer is now unexpectedly converted to thresholding_hls. I fixed this here: eki-project@9d4c654

Note that 1) an additional AbsorbConsecutiveTransposes() was needed and 2) InferConvInpGen() had to be moved to not leave 2x Im2Col in front of the pooling layers (this adjustment was probably missed during the recent consolidation of the pooling operators).

I wonder if this will have negative/unexpected implications for other models used in examples/regression tests...

2) test_fifosizing_linear

Error encountered in minimize_accumulator_width:

Exception: Could not find a suitable int datatype for -3.4028234663852886e+38

I fixed this by enforcing FP32 datatype for thresholds in case of FP32 inputs: eki-project@1e99b39

3) test_split_large_fifos

As with the cnv, the model now has 1 additional thresholding layer + DWC, leading to 2 additional FIFOs that were not accounted for. Fixed here: eki-project@d72df88

@fpjentzsch
Copy link
Collaborator

For some models, like the KWS example, this also uncovers streamlining problems that we should probably address with proper layout conversion in the front-end and more robust handling of Transpose/Reshape/Flatten operators. See eki-project#101

@auphelia auphelia mentioned this pull request Sep 22, 2025
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.

4 participants