-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
py/misc: use __builtin_strcmp
to enable compile-time optimization
#17196
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
base: master
Are you sure you want to change the base?
Conversation
(cherry picked from commit e2bba3c)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17196 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 169 169
Lines 21890 21890
=======================================
Hits 21572 21572
Misses 318 318 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code size report:
|
The way I read that comment, it is talking about having to do a bunch of
The code size report says this didn't change the code size on any of the common ports, so it seems like this change has no effect. Is it possible you are compiling with different optimization settings? |
In GCC from version 4.8 (the oldest used to build current MicroPython, for the ESP8266 platform) and onwards, I believe LLVM/Clang behaves the same way, so that wouldn't change much on macOS or Android builds too. Not sure about MSVC, it may have an intrinsic that wraps an x86/x64 opcode to perform that operation, but with a different name. Also, If you really want to stick with this then you should wrap this in a macro or do a bit of C preprocessing work beforehand, see how |
Thanks! I will compare the optimization flags. |
Our build is using
And IIUC,
|
Generally, MicroPython uses just |
(cherry picked from commit e2bba3c)
Summary
Otherwise, it seems that the compiler doesn't optimize the
strcmp
generated here:https://github.com/dpgeorge/micropython/blob/154b4eb354d97c7c28253bdc5212b2e58ea6462e/py/misc.h#L286
This issue was reported in #7659 (comment) and encountered when enabling
MICROPY_ROM_TEXT_COMPRESSION
in trezor/trezor-firmware#4961.Following this discussion, I have tried to replace
strcmp
by__builtin_strcmp
and it seems to work as intended.Testing
Tested on a simple function:
using
After the fix
Before the fix