[PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode

Adhemerval Zanella-2
Current optimized armv7 neon memchr uses the NO_THUMB wrongly to
conditionalize thumb instruction usage.  The flags is meant to be
defined before sysdep.h inclusion and to indicate the assembly
requires to build in ARM mode, not to check whether thumb is
enable or not.  This patch fixes it by using the GCC provided
'__thumb__' instead.

Also, even if the implementation is fixed to not use thumb instructions
it was clearly not proper checked in ARM mode: the carry bit flag will
be reset in previous 'cmp synd, #0' and thus the 'bhi cntin, #0' won't
be able to branch correctly if the loop finishes with 'cntin' being
negative (indicating that some bytes still require to be checked).
This patch also fixes it by checking the carry flag in previous loop
iteration directly (in ARM mode it will run both '.Lmasklast' and
'.Ltail' even if no byte is found in last loop iteration).

Checked on arm-linux-gnueabihf (with -marm and -mthumb mode).

        [BZ #23031]
        * sysdeps/arm/armv7/multiarch/memchr_neon.S (memchr): Fix tail check
        on ARM mode.
        (NO_THUMB): Check __thumb__ instead.
---
 ChangeLog                                 | 7 +++++++
 sysdeps/arm/armv7/multiarch/memchr_neon.S | 9 +++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/sysdeps/arm/armv7/multiarch/memchr_neon.S b/sysdeps/arm/armv7/multiarch/memchr_neon.S
index 1b2ae75..1b2a69d 100644
--- a/sysdeps/arm/armv7/multiarch/memchr_neon.S
+++ b/sysdeps/arm/armv7/multiarch/memchr_neon.S
@@ -68,7 +68,7 @@
  * allows to identify exactly which byte has matched.
  */
 
-#ifndef NO_THUMB
+#ifdef __thumb__
  .thumb_func
 #else
  .arm
@@ -132,7 +132,7 @@ ENTRY(memchr)
  /* The first block can also be the last */
  bls .Lmasklast
  /* Have we found something already? */
-#ifndef NO_THUMB
+#ifdef __thumb__
  cbnz synd, .Ltail
 #else
  cmp synd, #0
@@ -176,14 +176,11 @@ ENTRY(memchr)
  vpadd.i8 vdata0_0, vdata0_0, vdata1_0
  vpadd.i8 vdata0_0, vdata0_0, vdata0_0
  vmov synd, vdata0_0[0]
-#ifndef NO_THUMB
+#ifdef __thumb__
  cbz synd, .Lnotfound
  bhi .Ltail /* Uses the condition code from
    subs cntin, cntin, #32 above.  */
 #else
- cmp synd, #0
- beq .Lnotfound
- cmp cntin, #0
  bhi .Ltail
 #endif
 
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4] arm: Fix armv7 neon strcmp on ARM mode

Adhemerval Zanella-2
Current optimized armv7 neon strcmp uses the NO_THUMB wrongly to
conditionalize thumb instruction usage.  The flags is meant to be
defined before sysdep.h inclusion and to indicate the assembly
requires to build in ARM mode, not to check whether thumb is
enable or not.  This patch fixes it by using the GCC provided
'__thumb__' instead.

Also, even if the implementation is fixed to not use thumb instructions
it was clearly not proper checked in ARM mode: the 'prepare_mask' does
not build (it sets the 'mvn' instruction to use register predicate in
shift amount).  This patch fixes it by using a S2HI plus mvn to mimic
the expected nor operation.

Checked on arm-linux-gnueabihf (with -marm and -mthumb mode).

        [BZ #23031]
        * sysdeps/arm/armv7/strcmp.S [!__thumb__] (prepare_mask): Fix build
        and logic.
---
 ChangeLog                  | 4 ++++
 sysdeps/arm/armv7/strcmp.S | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/sysdeps/arm/armv7/strcmp.S b/sysdeps/arm/armv7/strcmp.S
index 060b865..a20b3e5 100644
--- a/sysdeps/arm/armv7/strcmp.S
+++ b/sysdeps/arm/armv7/strcmp.S
@@ -82,8 +82,7 @@
 #define data2 r3
 #define syndrome tmp2
 
-
-#ifndef NO_THUMB
+#ifdef __thumb__
 /* This code is best on Thumb.  */
  .thumb
 
@@ -97,7 +96,8 @@
 #else
 /* In ARM code we don't have ORN, but we can use MVN with a register shift.  */
 .macro prepare_mask mask_reg, nbits_reg
- mvn \mask_reg, const_m1, S2HI \nbits_reg
+ S2HI \mask_reg, const_m1, \nbits_reg
+ mvn \mask_reg, \mask_reg
 .endm
 .macro apply_mask data_reg, mask_reg
  orr \data_reg, \data_reg, \mask_reg
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/4] arm: Enable ARM mode for armv6 memchr

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
Current optimized armv6t2 memchr uses the NO_THUMB wrongly to
conditionalize thumb instruction usage.  The flags is meant to be
defined before sysdep.h inclusion and to indicate the assembly
requires to build in ARM mode, not to check whether thumb is
enable or not.  This patch fixes it by using the GCC provided
'__thumb__' instead.

Checked on arm-linux-gnueabihf (with -marm -march=armv6t2).

        * sysdeps/arm/armv6t2/memchr.S (NO_THUMB): Check for __thumb__
        instead.
---
 ChangeLog                    |  3 +++
 sysdeps/arm/armv6t2/memchr.S | 10 +++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/sysdeps/arm/armv6t2/memchr.S b/sysdeps/arm/armv6t2/memchr.S
index bdd385b..03b7f32 100644
--- a/sysdeps/arm/armv6t2/memchr.S
+++ b/sysdeps/arm/armv6t2/memchr.S
@@ -42,7 +42,7 @@
  .syntax unified
 
  .text
-#ifdef NO_THUMB
+#ifndef __thumb__
  .arm
 #else
  .thumb
@@ -91,7 +91,7 @@ ENTRY(memchr)
 
 15:
  ldrd r4,r5, [r0],#8
-#ifndef NO_THUMB
+#ifdef __thumb__
  subs r6, r6, #8
 #endif
  eor r4,r4, r1 @ Get it so that r4,r5 have 00's where the bytes match the target
@@ -100,7 +100,7 @@ ENTRY(memchr)
  sel r4, r3, r7 @ bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
  uadd8 r5, r5, r7 @ Parallel add 0xff - sets the GE bits for anything that wasn't 0
  sel r5, r4, r7 @ chained....bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
-#ifndef NO_THUMB
+#ifdef __thumb__
  cbnz r5, 60f
 #else
  cmp r5, #0
@@ -120,7 +120,7 @@ ENTRY(memchr)
  and r2,r2,#7 @ Leave the count remaining as the number after the double words have been done
 
 20:
-#ifndef NO_THUMB
+#ifdef __thumb__
  cbz r2, 40f @ 0 length or hit the end already then not found
 #else
  cmp r2, #0
@@ -129,7 +129,7 @@ ENTRY(memchr)
 
 21:  @ Post aligned section, or just a short call
  ldrb r3,[r0],#1
-#ifndef NO_THUMB
+#ifdef __thumb__
  subs r2,r2,#1
  eor r3,r3,r1 @ r3 = 0 if match - doesn't break flags from sub
  cbz r3, 50f
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
Current optimized armv6t2 strlen uses the NO_THUMB wrongly to
conditionalize thumb instruction usage.  The flags is meant to be
defined before sysdep.h inclusion and to indicate the assembly
requires to build in ARM mode, not to check whether thumb is
enable or not.  This patch fixes it by using the GCC provided
'__thumb__' instead.

Checked on arm-linux-gnueabihf (with -marm -march=armv6t2).

        * sysdeps/arm/armv6t2/strlen.S (NO_THUMB): Check for __thumb__
        instead.
---
 ChangeLog                    | 3 +++
 sysdeps/arm/armv6t2/strlen.S | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/sysdeps/arm/armv6t2/strlen.S b/sysdeps/arm/armv6t2/strlen.S
index 6988183..f4111c3 100644
--- a/sysdeps/arm/armv6t2/strlen.S
+++ b/sysdeps/arm/armv6t2/strlen.S
@@ -21,7 +21,7 @@
 
  */
 
-#include <arm-features.h>               /* This might #define NO_THUMB.  */
+#include <arm-features.h>
 #include <sysdep.h>
 
 #ifdef __ARMEB__
@@ -32,7 +32,7 @@
 #define S2HI lsl
 #endif
 
-#ifndef NO_THUMB
+#ifdef __thumb__
 /* This code is best on Thumb.  */
  .thumb
 #else
@@ -146,7 +146,7 @@ ENTRY(strlen)
  tst tmp1, #4
  pld [src, #64]
  S2HI tmp2, const_m1, tmp2
-#ifdef NO_THUMB
+#ifndef __thumb__
  mvn tmp1, tmp2
  orr data1a, data1a, tmp1
  itt ne
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Wed, 2018-04-11 at 18:16 -0300, Adhemerval Zanella wrote:
> Current optimized armv6t2 strlen uses the NO_THUMB wrongly to
> conditionalize thumb instruction usage.  The flags is meant to be
> defined before sysdep.h inclusion and to indicate the assembly
> requires to build in ARM mode, not to check whether thumb is
> enable or not.  This patch fixes it by using the GCC provided
> '__thumb__' instead.

Is it ever useful to build for ARM-state when on armv6t2 (i.e. Thumb2
is guaranteed to be available)?  It's not totally obvious from reading
the source that the ARM version is going to be better in any way than
the Thumb one.

Assuming that this is indeed something worth supporting, I think the
short log for the patch ought to say "armv6t2" not "armv6".

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 11/04/2018 19:12, Phil Blundell wrote:

> On Wed, 2018-04-11 at 18:16 -0300, Adhemerval Zanella wrote:
>> Current optimized armv6t2 strlen uses the NO_THUMB wrongly to
>> conditionalize thumb instruction usage.  The flags is meant to be
>> defined before sysdep.h inclusion and to indicate the assembly
>> requires to build in ARM mode, not to check whether thumb is
>> enable or not.  This patch fixes it by using the GCC provided
>> '__thumb__' instead.
>
> Is it ever useful to build for ARM-state when on armv6t2 (i.e. Thumb2
> is guaranteed to be available)?  It's not totally obvious from reading
> the source that the ARM version is going to be better in any way than
> the Thumb one.

I guess not, but even though the implementation allows it and the flag
usage is wrong.  Another option would just remove ARM code, but I think
for this we will need to also add configure check to require thumb as
well.

>
> Assuming that this is indeed something worth supporting, I think the
> short log for the patch ought to say "armv6t2" not "armv6".

Right, I changed it locally.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote:
> I guess not, but even though the implementation allows it and the
> flag usage is wrong.  Another option would just remove ARM code, but
> I think for this we will need to also add configure check to require
> thumb as well.

If the ARM implementation has no benefit (i.e. is no faster than the
Thumb one but takes the same/more space) then I think we should delete
it.  Having two versions of the code just seems like an unnecessary
maintenance headache and source of bugs.

What would the configure check be testing for?  If the target arch is
set to armv6t2 (which it must be if we're using this file) then, by
definition, Thumb is available.  And the fact that we've apparently
been always building the Thumb version in the past seems like a further
indication that the use of Thumb here is not a problem.

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 12/04/2018 12:33, Phil Blundell wrote:

> On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote:
>> I guess not, but even though the implementation allows it and the
>> flag usage is wrong.  Another option would just remove ARM code, but
>> I think for this we will need to also add configure check to require
>> thumb as well.
>
> If the ARM implementation has no benefit (i.e. is no faster than the
> Thumb one but takes the same/more space) then I think we should delete
> it.  Having two versions of the code just seems like an unnecessary
> maintenance headache and source of bugs.

Agreed.

>
> What would the configure check be testing for?  If the target arch is
> set to armv6t2 (which it must be if we're using this file) then, by
> definition, Thumb is available.  And the fact that we've apparently
> been always building the Thumb version in the past seems like a further
> indication that the use of Thumb here is not a problem.

I sent the email before I realized armv6t2 always imply in thumb. I think
for 3rd and 4th patch a better approach would be just to remove the ARM
path and have one version, I will update them.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 12/04/2018 13:16, Adhemerval Zanella wrote:

>
>
> On 12/04/2018 12:33, Phil Blundell wrote:
>> On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote:
>>> I guess not, but even though the implementation allows it and the
>>> flag usage is wrong.  Another option would just remove ARM code, but
>>> I think for this we will need to also add configure check to require
>>> thumb as well.
>>
>> If the ARM implementation has no benefit (i.e. is no faster than the
>> Thumb one but takes the same/more space) then I think we should delete
>> it.  Having two versions of the code just seems like an unnecessary
>> maintenance headache and source of bugs.
>
> Agreed.
>
>>
>> What would the configure check be testing for?  If the target arch is
>> set to armv6t2 (which it must be if we're using this file) then, by
>> definition, Thumb is available.  And the fact that we've apparently
>> been always building the Thumb version in the past seems like a further
>> indication that the use of Thumb here is not a problem.
>
> I sent the email before I realized armv6t2 always imply in thumb. I think
> for 3rd and 4th patch a better approach would be just to remove the ARM
> path and have one version, I will update them.
>

In fact for strlen we still require to provide a ARM only mode, since
armv7 build will select this implementation as default and it still
possible the user would require a ARM only build.  
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Thu, 2018-04-12 at 16:20 -0300, Adhemerval Zanella wrote:
> In fact for strlen we still require to provide a ARM only mode, since
> armv7 build will select this implementation as default and it still
> possible the user would require a ARM only build.  

ARMv7 guarantees Thumb2 is available, doesn't it?

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 12/04/2018 16:32, Ramana Radhakrishnan wrote:

>
>
> On Thu, 12 Apr 2018, 20:28 Phil Blundell, <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On Thu, 2018-04-12 at 16:20 -0300, Adhemerval Zanella wrote:
>     > In fact for strlen we still require to provide a ARM only mode, since
>     > armv7 build will select this implementation as default and it still
>     > possible the user would require a ARM only build.  
>
>     ARMv7 guarantees Thumb2 is available, doesn't it?
>
>
> Yep. I am puzzled as to why this patchset is.needed .
>
> Ramana

Because as reported by BZ#23031 [1], the user is using a kernel explicit
configured to no enable thumb instructions. Although this kernel config
is debatable whether is brings any gain to userland, it still a valid
one.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=23031
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Thu, 2018-04-12 at 16:41 -0300, Adhemerval Zanella wrote:
> Because as reported by BZ#23031 [1], the user is using a kernel
> explicit
> configured to no enable thumb instructions. Although this kernel
> config 
> is debatable whether is brings any gain to userland, it still a valid
> one.

I don't think it's legitimate for the user to tell glibc that he's
using ARMv7 and then configure the runtime environment to disable some
parts of that architecture.  If he doesn't want Thumb, he should
configure glibc for an architecture that doesn't include it.

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 12/04/2018 17:31, Phil Blundell wrote:

> On Thu, 2018-04-12 at 16:41 -0300, Adhemerval Zanella wrote:
>> Because as reported by BZ#23031 [1], the user is using a kernel
>> explicit
>> configured to no enable thumb instructions. Although this kernel
>> config 
>> is debatable whether is brings any gain to userland, it still a valid
>> one.
>
> I don't think it's legitimate for the user to tell glibc that he's
> using ARMv7 and then configure the runtime environment to disable some
> parts of that architecture.  If he doesn't want Thumb, he should
> configure glibc for an architecture that doesn't include it.
>
> p.

Even though it configure the toolchain to not emit thumb, if user tries
to optimize for armv7 (by tinkering with CC or CFLAGS) it will still
emit thumb instructions because of the optimized assembly implementations.
And the expectation imho is to if user explicit builds with -marm the
resulting library should not user thumb instructions.

In fact, the whole idea of current code is indeed to prevent thumb
instructions in such cases, it is just using the *wrong* preprocessor
to check it (and the  wrong assumption from the arm-features.h
include kinda confirm it).


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote:
> Even though it configure the toolchain to not emit thumb, if user
> tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will
> still emit thumb instructions because of the optimized assembly
> implementations.
> And the expectation imho is to if user explicit builds with -marm
> the resulting library should not user thumb instructions.

I think the precedent on other architectures is that glibc will use the
instruction set appropriate to the target triple it was given.  For
example, if you configure glibc for i686-linux-gnu then it will use
CMOV instructions, and setting -march=i586 in CFLAGS won't prevent
this.

I continue to feel that the scenario mentioned in the bug report you
linked to (configuring for armv7 but disabling Thumb in the kernel) is
just silly and we should not be indicating to users that this is
supported.  T32 is an integral part of ARMv7 (indeed, the M profile
doesn't support A32 at all) and if you take it out then the resulting
architecture is no longer ARMv7.  It seems undesirable to force all the
ARMv7-optimised assembly routines to also provide an ARM-only version
even if the resulting performance is the same or worse than the Thumb
implementation.  This code is never going to get tested in practice
(witness the fact that you found it's been broken for several years)
and it's just a liability.

> In fact, the whole idea of current code is indeed to prevent thumb
> instructions in such cases

That's true.  It's not entirely clear to me why Roland made that change
 in the first place but I think it was roughly contemporaneous with the
NaCl port and I'm sort of guessing it was something to do with that.
Does anybody else know/remember?

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 13/04/2018 06:56, Phil Blundell wrote:

> On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote:
>> Even though it configure the toolchain to not emit thumb, if user
>> tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will
>> still emit thumb instructions because of the optimized assembly
>> implementations.
>> And the expectation imho is to if user explicit builds with -marm
>> the resulting library should not user thumb instructions.
>
> I think the precedent on other architectures is that glibc will use the
> instruction set appropriate to the target triple it was given.  For
> example, if you configure glibc for i686-linux-gnu then it will use
> CMOV instructions, and setting -march=i586 in CFLAGS won't prevent
> this.

In fact to determine the sysdeps folders glibc build what machine the
compiler is configured for.  On ARM for instance, the base machine is
determined at sysdeps/arm/preconfigure.ac by checking the __ARM_ARCH_*
builtin preprocessor direct from compiler. So it does not really matter
if use armv7-linux-gnueabihf is used as triple, since the preprocessor
builtint can be changed by -march.

And I think this is the correct way, the multiarch idea is exactly to
avoid the necessity of explicit set the target ISA.  And user can also
tune if required by changing the CC/CFLAGS for the desirable target.

>
> I continue to feel that the scenario mentioned in the bug report you
> linked to (configuring for armv7 but disabling Thumb in the kernel) is
> just silly and we should not be indicating to users that this is
> supported.  T32 is an integral part of ARMv7 (indeed, the M profile
> doesn't support A32 at all) and if you take it out then the resulting
> architecture is no longer ARMv7.  It seems undesirable to force all the
> ARMv7-optimised assembly routines to also provide an ARM-only version
> even if the resulting performance is the same or worse than the Thumb
> implementation.  This code is never going to get tested in practice
> (witness the fact that you found it's been broken for several years)
> and it's just a liability.

Since T32 is an integral part of ARMv7, I would expect either that kernel
do not provide an option to disable it or at least emulate thumb instruction
if underlying hardware do not provide it (as for other various
architectures for some atomic operation for instance). However the current
scenario exists and I see no strong reason to not support it.

We already handle the cases in generic code where we should not generate
thumb (NO_THUMB macro) and the fixes I sent already are minimal.  For
newer implementation it is a matter of if the idea is to always support
thumb so simple redirection to a previous implementation is straightforward
(ifndef __thumb__ then include old implementation).

But I give you this incurs in more maintainability and I do agree we should
avoid such path. However what we shouldn't is simply breaking at runtime
due a non-supported configuration. If the idea is to support thumb as default,
we should then indicate at build time that it is required (either at configure
time or at build time).

>
>> In fact, the whole idea of current code is indeed to prevent thumb
>> instructions in such cases
>
> That's true.  It's not entirely clear to me why Roland made that change
>  in the first place but I think it was roughly contemporaneous with the
> NaCl port and I'm sort of guessing it was something to do with that.
> Does anybody else know/remember?
>
> p.
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Ramana Radhakrishnan-7
In reply to this post by Phil Blundell-3
On Fri, Apr 13, 2018 at 10:56 AM, Phil Blundell <[hidden email]> wrote:

> On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote:
>> Even though it configure the toolchain to not emit thumb, if user
>> tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will
>> still emit thumb instructions because of the optimized assembly
>> implementations.
>> And the expectation imho is to if user explicit builds with -marm
>> the resulting library should not user thumb instructions.
>
> I think the precedent on other architectures is that glibc will use the
> instruction set appropriate to the target triple it was given.  For
> example, if you configure glibc for i686-linux-gnu then it will use
> CMOV instructions, and setting -march=i586 in CFLAGS won't prevent
> this.
>
> I continue to feel that the scenario mentioned in the bug report you
> linked to (configuring for armv7 but disabling Thumb in the kernel) is
> just silly and we should not be indicating to users that this is
> supported.  T32 is an integral part of ARMv7 (indeed, the M profile
> doesn't support A32 at all) and if you take it out then the resulting
> architecture is no longer ARMv7.  It seems undesirable to force all the
> ARMv7-optimised assembly routines to also provide an ARM-only version
> even if the resulting performance is the same or worse than the Thumb
> implementation.  This code is never going to get tested in practice
> (witness the fact that you found it's been broken for several years)
> and it's just a liability.

>
>> In fact, the whole idea of current code is indeed to prevent thumb
>> instructions in such cases
>
> That's true.  It's not entirely clear to me why Roland made that change
>  in the first place but I think it was roughly contemporaneous with the
> NaCl port and I'm sort of guessing it was something to do with that.
> Does anybody else know/remember?

That change coming in with the NaCl port makes sense to me. IIRC NaCl
wanted fixed length encodings, (thus )everything to be in Arm state
only : instructions that were "forbidden" / not allowed as part of the
whole NaCl strategy for providing sand boxes including switching to
Thumb. I also seem to remember that they had stuff that checked
instructions in the binary were in a particular set and anything
outside was not considered to be a safe binary.

Aha - https://developer.chrome.com/native-client/reference/sandbox_internals/arm-32-bit-sandbox#arm-32-bit-sandbox



regards
Ramana


> p.
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Fri, 2018-04-13 at 13:06 +0100, Ramana Radhakrishnan wrote:

> That change coming in with the NaCl port makes sense to me. IIRC NaCl
> wanted fixed length encodings, (thus )everything to be in Arm state
> only : instructions that were "forbidden" / not allowed as part of
> the
> whole NaCl strategy for providing sand boxes including switching to
> Thumb. I also seem to remember that they had stuff that checked
> instructions in the binary were in a particular set and anything
> outside was not considered to be a safe binary.
>
> Aha - https://developer.chrome.com/native-client/reference/sandbox_in
> ternals/arm-32-bit-sandbox#arm-32-bit-sandbox

Ah, thanks, that makes sense.  In that case, given that the rest of the
NaCl port was removed a year ago, maybe we should also remove this
cruft.  I think that'd essentially mean reverting
4f510e3aeeeb3fd974a12a71789fa9c63ab8c6dd and similar commits.

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 13/04/2018 09:44, Phil Blundell wrote:

> On Fri, 2018-04-13 at 13:06 +0100, Ramana Radhakrishnan wrote:
>> That change coming in with the NaCl port makes sense to me. IIRC NaCl
>> wanted fixed length encodings, (thus )everything to be in Arm state
>> only : instructions that were "forbidden" / not allowed as part of
>> the
>> whole NaCl strategy for providing sand boxes including switching to
>> Thumb. I also seem to remember that they had stuff that checked
>> instructions in the binary were in a particular set and anything
>> outside was not considered to be a safe binary.
>>
>> Aha - https://developer.chrome.com/native-client/reference/sandbox_in
>> ternals/arm-32-bit-sandbox#arm-32-bit-sandbox
>
> Ah, thanks, that makes sense.  In that case, given that the rest of the
> NaCl port was removed a year ago, maybe we should also remove this
> cruft.  I think that'd essentially mean reverting
> 4f510e3aeeeb3fd974a12a71789fa9c63ab8c6dd and similar commits.
>
> p.
>

I am not against reverting it, but it does not help the scenario of
BZ#23031.  We need to define whether environments which thumb is
expected to be supported but it is not enabled by compiler flags
(-marm) and act accordingly by avoid building glibc in such cases.

If we define that is the desirable case, we can also cleanup the
ARM code path for armv7 memchr and strcmp.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Fri, 2018-04-13 at 11:40 -0300, Adhemerval Zanella wrote:
> I am not against reverting it, but it does not help the scenario of
> BZ#23031.  We need to define whether environments which thumb is
> expected to be supported but it is not enabled by compiler flags
> (-marm) and act accordingly by avoid building glibc in such cases.

My position on that remains that the scenario of BZ#23031 is simply
invalid and the user's expectations are just misplaced.  The fact that
we've had sysdeps/arm/armv6t2/memchr.S using Thumb code since December
2011 and in those six years it's only generated one bug report seems to
support the belief that there is not a large population of users trying
to do this sort of thing.

I don't think -marm has ever been intended to mean "don't allow any
Thumb instructions in my program".  The whole -marm/-mthumb thing dates
from ARMv4T/Thumb-1 days when interworking couldn't be taken for
granted and Thumb code often ran significantly slower than the
equivalent ARM code.  Under those circumstances it was necessary for
the user to be able to choose what instruction encoding the compiler
was going to generate.

But here we are, two decades later, and the landscape is a bit
different.  It's not entirely clear that -marm/-mthumb are very useful
options in an ARMv7 world because the user shouldn't need to care.  In
an ideal world the compiler would be able to predict for any given
function whether T32 or A32 encoding would give the best results
(either speed or space according to the selected optimisation settings)
 and proceed accordingly.  Unfortunately in practice I don't think
we're quite there yet and for critical code there's still an element of
"try building it both ways round and see which one runs quickest" which
means the compiler flags probably are still necessary.  Users might
even legitimately wish to try that experiment with glibc, so forbidding
them to compile it under -marm would not obviously be the right thing
to do.

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Wilco Dijkstra-2
In reply to this post by Adhemerval Zanella-2
Hi,

I can't see a valid reason for disabling Thumb-2 when it is available - it's not slower
and if necessary you can align loops or force Thumb-2 instructions to be 32 bit in an inner
loop. So basically if you have Thumb-2 available you might as well use it to get smaller
code - there is no advantage in using Arm. Supporting a no-Thumb-2 build would be
additional maintenance (unless it was an official variant it would not be tested as you've
shown, and I think there are already too many variants...).

Also keeping assembly code clean without lots of #if's that make it hard to follow the
logic (or the carry flags!) is best.

Wilco
12