-
Notifications
You must be signed in to change notification settings - Fork 2k
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
pkg/wamr: Added support for THUMB_VFP in wamr Makefile #20628
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.
Change looks good, you want to follow the commit title convention though, so plaese change the commit message to something like
pkg/wamr: add support for THUMB_VFP
You can do so with git rebase -i HEAD~2
- then you can also squash your commits.
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.
Thank you for the patch!
@@ -44,7 +44,11 @@ ifeq ($(findstring aarch,$(OS_ARCH)),aarch) | |||
WAMR_BUILD_TARGET = ARM | |||
endif | |||
else ifeq ($(findstring arm,$(CPU_ARCH)),arm) | |||
WAMR_BUILD_TARGET = THUMB | |||
ifeq ($(findstring cortexm_fpu,$(FEATURES_PROVIDED)),cortexm_fpu) |
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.
Nit picking: Would it be possible to disable hardware FPU support if that is provided, and is this something that we need to be prepared for?
I think just using FEATURES_USED
instead of FEATURES_PROVIDED
would work. But there is a CI test that will fail if anyone uses the FEAUTRES_USED
variable, which would prevent this from getting merged. I wonder if that CI check has outlived its usefulness anyway.
In either case, I think merging this is an improvement and this concern should not block this. But maybe we could add a follow up, if there is any merit to my concern.
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.
Regarding the FEATURES_USED
vs FEATURES_PROVIDED
, since from my understanding VFP is an extension and therefore provides the same capabilities as the normal architecture I think that as long it doesn't create performance issues it should be used if the feature is provided.
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.
Note that the ABI between hardfp and softfp is not compatible. You can safely run softfp code on a hardfpu MCU (if floats are actually used softfp will be both slower and larger, though). But you cannot link objects compiled for a softfp ABI together with objects compiled for a hardfp ABI.
Contribution description
Edited wamr Makefile to support THUMB_VFP
Testing procedure
Build and test with arduino-nano-33-ble (nrf52840 cpu) for THUMB_VFP mode and built with nrf51dk for THUMB mode.
Issues/PRs references