bn_mul.h: require at least ARMv6 to enable the ARM DSP code #2777
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
Commit 16b1bd8 "bn_mul.h: add ARM DSP optimized MULADDC code" added some ARM DSP instructions that was assumed to always be available when __ARM_FEATURE_DSP is defined to 1. Unfortunately it appears that the ARMv5TE architecture (GCC flag -march=armv5te) supports the DSP instructions, but only in Thumb mode and not in ARM mode, despite defining __ARM_FEATURE_DSP in both cases. This patch fixes the build issue by requiring at least ARMv6 in addition to the DSP feature.
Non-regression test for "bn_mul.h: require at least ARMv6 to enable the ARM DSP code"
gilles-peskine-arm
added
bug
CLA valid
needs-review
Without any -O option, the default is -O0, and then the assembly code is not used, so this would not be a non-regression test for the assembly code that doesn't build.
gilles-peskine-arm
force-pushed
the
bn_mul-arm-dsp-fix-development
branch
from
be0a059 to
8a52af9
Compare
|
There was a bug in the non-regression test which was fixed in 8a52af9, which was to do with compiler optimization flag not set. The test was passing with the fix - 5daa34f, which shouldn't be the case Further, on the investigation, there were a couple of non-regression tests which were not using the optimization flags, but I strongly think this be0a059 needs to be raised as new PR. I'm happy for this PR to be merged. |
jainvikas8
previously approved these changes
Aug 8, 2019
AndrzejKurek
previously approved these changes
Aug 9, 2019
k-stachowiak
previously approved these changes
Aug 9, 2019
Call the component xxx_arm5vte, because that's what it does. Explain "armel", and more generally why this component exists, in a comment.
gilles-peskine-arm
dismissed stale reviews from k-stachowiak, AndrzejKurek, and jainvikas8
via
2c897d7
k-stachowiak
approved these changes
Aug 12, 2019
AndrzejKurek
approved these changes
Aug 13, 2019
gilles-peskine-arm
removed
the
needs-backports
gilles-peskine-arm
added
approved
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.
Fix the build on ARMv5TE in ARM mode. The assembly code in
bn_mul.huses DSP instructions that are not available on this particular architecture, so set the conditional compilation directives to exclude them.Fix originally contributed in #2169 by @aurel32. I rebased to current development to facilitate testing and added a changelog entry and a non-regression test.
Needs backport to 2.16: #2778. The offending code is not present in 2.7.