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

Add expression balancing for xilinxhls backend #68

Merged
merged 2 commits into from
May 13, 2024

Conversation

francescobrivio
Copy link
Contributor

PR Description

This PR is addressing issue #66.
Main changes:

  • In commit 099599d I am implementing a tree reduction method similar to what is done in hlt4ml (see these lines)
  • The method is used when summing the tree scores in backends/xilinxhls/firmware/BDT_unrolled.h
  • For this to work I had to reverse the indexes of the score_t scores array, from:
    score_t scores[n_trees][fn_classes(n_classes)];
    
    to
    score_t scores[fn_classes(n_classes)][n_trees];
    

These changes bring a notable improvements in latency (and partially in resource consumption) when using the AP_SAT flag in the BDT ScorePrecision, while maintaining performances identical to the current version of conifer in terms of BDT score.

Additionally, in commit bffd3a6 I fixed a typo in the "Development instructions" and added pandas to the dev_requirements.txt file.

PR Validation

The following tests are done with:

  • xgboost v1.7.5
  • Vitis_LHS v2023.2
  • Binary classification BDT with 327 trees and max_depth = 4

Latency/Resources table:

Conifer version ScorePrecision vsynth LUT vsynth FF Latency
master ap_fixed<11,4,AP_RND_CONV,AP_SAT> 51936 5032 133
This PR ap_fixed<11,4,AP_RND_CONV,AP_SAT> 51163 4489 9
-------- -------- -------- -------- --------
master ap_fixed<11,4,AP_RND_CONV> 43329 3542 4
This PR ap_fixed<11,4,AP_RND_CONV> 42988 3800 4

Score plots:

@thesps
Copy link
Owner

thesps commented May 13, 2024

Thanks for this nice contribution! I've checked on some other models and see consistent improvements as you have shown. The tests are passing too (well, as much as they are passing on master branch...).

I will therefore merge this!

@thesps thesps merged commit a36ccde into thesps:master May 13, 2024
@thesps thesps mentioned this pull request May 13, 2024
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.

2 participants