-
Notifications
You must be signed in to change notification settings - Fork 20
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
Perceptron predictor #374
Perceptron predictor #374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation on the whole looks great. Just some comments on clarity and naming, again for clarity
Please can you update the documentation with the new config options and a small section about how the new predictor works |
I was just wondering if it would be possible to add the sources (research papers/ websites) you've used to carry out the implementation. This could help reviewers and is a good piece of documentation for future changes. |
I suspect these tests were run in debug mode. It's great that you have lots of results, but it may be worth doing a couple comparative spot checks in release mode to ensure that the speedup is consistent between the two (misprediction rate should remain the same), as this is the mode that performance will matter the most. No need to rerun all the tests unless differences are noticed, or if this is already in release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, nice work.
A couple small comments, although others' reviews mostly encapsulate changes that need to happen before approval.
6f71505
to
7cf73cd
Compare
Adding tests for perceptron predictor comparable to generic predictor tests, and amending existing tests which are effected by the changes to the BP config structure
43d94b0
to
4351315
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good now. Nice work, and nice performance improvements 👍
5f2b5b8
Introducing a new PerceptronPredictor class as an alternative to the GenericPredictor currently used in all the config files. Shows an improvement in branch prediction rate for all the benchmarks except for STREAM, which was already being well predicted by the Generic predictor. There is more work done by this predictor per prediction so it is marginally slower per prediction than the GenericPredictor, but it compensates for this by its improved accuracy meaning that SimEng was faster with it for all but 15 of the benchmarks (72). These 15 benchmarks are generally already very quick and the regression I observed was at most 3.6% compared to improvements of up to 35% for the other benchmarks. Average improvements of -5% runtime (percentage) and -8.5% mispredictions (raw).