apps: microtvm: Disable CONFIG_FPU for Zephyr runtime
#8055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@microbuilder Hi Kevin. Thanks a lot for addressing those annoying warnings related to I hope CI will catch it too, but it can take a while to do so, hence I'm sharing it here with you. You can reproduce it with locally by running, for instance: So, even if that works, I'm wondering how to disable it for the MPS2 board only, since CONFIG_FPU=y will be enabled by default anyway for all the boards, and apparently CONFIG_FPU=n is not enough to disable (override) it per board? |
|
@gromero I tested this on microtvm reference virtual machine with mps2_an521 platform and it was fine. |
|
I tested this with |
|
@microbuilder @mehrdadh Hi folks, sorry for the delay on reviewing it. @mehrdadh thanks for the additional checks! I figured out why the link error pasted above happened on my local environment but not on the CI. It happens that the CI is using Zephyr SDK 0.12 whilst I was using Zephyr SDK 0.11 and the following Zephyr SDK fix is only in 0.12: hence the soft-float functions were not present in my environment, causing the linking errors when the patch in question is applied. That, on the other hand, exposed the fact that but are included (and used) in the .elf image when the patch is applied (for This seems expected because although Thus one might think it was just a matter of enabling However although the build will finish ok the following error will be caught by the CI when testing against models that rely on floating point operations: This happens because microTVM-Zephyr build system ends up linking mixed objects, i.e. objects built with soft-float and without soft-float functions. This bug was recently fixed by PR #8230, which now allows to set Kevin, since Zephyr defaults regarding CONFIG_FPU might not be what microTVM needs (which is ok) I think we need to enabled FPU per board, for each board (@areusch and @mehrdadh are you ok with that?) So could you please update your patch? It also needs to be rebased since For the records, the "best-case scenario warning" about conflicts during builds mentioned by Kevin in his original commit message is the one that currently happens on the CI, i.e. [0]: |
gromero
requested changes
Jun 14, 2021
|
@microbuilder friendly ping :) |
|
@microbuilder please address @gromero's comment and rebase the PR |
microbuilder
force-pushed
the
zephyr_fpu
branch
3 times, most recently
from
522e8ab
to
67f358e
Compare
mehrdadh
approved these changes
Jul 15, 2021
|
@microbuilder Thanks! There is only a nit, a typo in the commit message that I'd like to get it fixed: |
`CONFIG_FPU` was being enabled by default for every platform, regardless of whether or not the platform using the sample app actually had a HW FPU unit. As a result, FPU instructions may be included on platforms that aren't able to support them, or in a best-case scenario we will get a warning about the conflict during builds, which pollutes the CI output, in a worst-case scenario a fault. This change removes the `CONFIG_FPU=y` setting from being set at the application level, since this flag should be set at the chip level for any platform that has an FPU. Signed-off-by: Kevin Townsend <[email protected]>
|
@microbuilder Thanks! LGTM! |
ylc
pushed a commit
to ylc/tvm
that referenced
this pull request
Sep 29, 2021
`CONFIG_FPU` was being enabled by default for every platform, regardless of whether or not the platform using the sample app actually had a HW FPU unit. As a result, FPU instructions may be included on platforms that aren't able to support them, or in a best-case scenario we will get a warning about the conflict during builds, which pollutes the CI output, in a worst-case scenario a fault. This change removes the `CONFIG_FPU=y` setting from being set at the application level, since this flag should be set at the chip level for any platform that has an FPU. Signed-off-by: Kevin Townsend <[email protected]>
zxy844288792
pushed a commit
to zxy844288792/tvm
that referenced
this pull request
Mar 4, 2022
`CONFIG_FPU` was being enabled by default for every platform, regardless of whether or not the platform using the sample app actually had a HW FPU unit. As a result, FPU instructions may be included on platforms that aren't able to support them, or in a best-case scenario we will get a warning about the conflict during builds, which pollutes the CI output, in a worst-case scenario a fault. This change removes the `CONFIG_FPU=y` setting from being set at the application level, since this flag should be set at the chip level for any platform that has an FPU. Signed-off-by: Kevin Townsend <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CONFIG_FPUwas being enabled by default for every platform,regardless of whether or not the platform using the sample app actually
had a HW FPU unit. As a result, FPU instructions may be included on
platforms that aren't able to support them, or in a best-case scenario
we will get a warning about the confllct during builds, which pollutes
the CI output, in a worst-case scenario a fault.
This change removes the
CONFIG_FPU=ysetting from being set at theapplication level, since this flag should be set at the chip level for
any platform that has an FPU.
Signed-off-by: Kevin Townsend [email protected]