-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
py/misc: Fix fallback implementation of mp_popcount. #17498
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
py/misc: Fix fallback implementation of mp_popcount. #17498
Conversation
@agatti FYI. Please can you double check this |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17498 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 169 169
Lines 21968 21968
=======================================
Hits 21654 21654
Misses 314 314 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code size report:
|
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.
I plugged this into cbmc and it is equivalent to a naive for-loop popcnt implementation for all 32-bit unsigned integers. The original formulation encounters a verification error.
I run the octoprobe tests against this PR: https://reports.octoprobe.org/github_selfhosted_testrun_76/octoprobe_summary_report.html These are the results: tests_mpy-cross_native.txt Now all md5 equal: The error has gone! |
I don't have access to any of my boards/dev machines until the end of the month to see what's going on there (along with my copy of "Hacker's Delight" to double check things against), but if this PR is reported by multiple sources to fix the issue then please go ahead! I took that code from an old project of mine running on a ColdFire board that's been working for a few years now, so now I wonder whether this works due to code consuming that value that either does the shift itself or masks it out and fills the other bits with something else - no idea until I take a look. |
Tested using gcc 7.3.1 which does not have the popcount built-in and uses this fallback version. Without the fix, mpy-cross produces mpy files with corrupt RISC-V machine code. With the fix, mpy-cross output is correct. Signed-off-by: Damien George <damien@micropython.org>
57d2fe8
to
6fee099
Compare
Summary
The fallback
mp_popcount()
was broken, missing a shift.Testing
Tested using gcc 7.3.1 which does not have the popcount built-in and uses this fallback version. Without the fix,
mpy-cross -march=rv32imc -X emit=native
produces mpy files with corrupt RISC-V machine code. With the fix,mpy-cross
output is correct.Note: gcc 7.3.1 can be tested using docker image
larsks/esp-open-sdk
.This bug was found as part of Octoprobe integration: octoprobe/testbed_micropython#27