diff mbox

[libgcc/ARM,1/6] Fix Thumb-1 only == ARMv6-M & Thumb-2 only == ARMv7-M assumptions

Message ID 4707964.lx3ShB1mLb@e108577-lin
State New
Headers show

Commit Message

Thomas Preudhomme June 27, 2016, 4:51 p.m. UTC 
Hi Ramana,

On Wednesday 01 June 2016 10:00:52 Ramana Radhakrishnan wrote:
> 
> From here down to ....
> 
> > -#if ((__ARM_ARCH__ > 5) && !defined(__ARM_ARCH_6M__)) \
> > -    || defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) \
> > -    || defined(__ARM_ARCH_5TEJ__)
> > -#define HAVE_ARM_CLZ 1
> > -#endif
> > -
> > 
> >  #ifdef L_clzsi2
> > 
> > -#if defined(__ARM_ARCH_6M__)
> > +#if !__ARM_ARCH_ISA_ARM && __ARM_ARCH_ISA_THUMB == 1
> > 
> >  FUNC_START clzsi2
> >  
> >         mov     r1, #28
> >         mov     r3, #1
> > 
> > @@ -1544,7 +1538,7 @@ FUNC_START clzsi2
> > 
> >         FUNC_END clzsi2
> >  
> >  #else
> >  ARM_FUNC_START clzsi2
> > 
> > -# if defined(HAVE_ARM_CLZ)
> > +# if defined(__ARM_FEATURE_CLZ)
> > 
> >         clz     r0, r0
> >         RET
> >  
> >  # else
> > 
> > @@ -1568,15 +1562,15 @@ ARM_FUNC_START clzsi2
> > 
> >  .align 2
> >  1:
> >  .byte 4, 3, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0
> > 
> > -# endif /* !HAVE_ARM_CLZ */
> > +# endif /* !__ARM_FEATURE_CLZ */
> > 
> >         FUNC_END clzsi2
> >  
> >  #endif
> >  #endif /* L_clzsi2 */
> >  
> >  #ifdef L_clzdi2
> > 
> > -#if !defined(HAVE_ARM_CLZ)
> > +#if !defined(__ARM_FEATURE_CLZ)
> 
> here should be it's own little patchlet and can  go in separately.

The patch in attachment changes the CLZ availability check in libgcc to test 
ISA supported and architecture version rather than encode a specific list of 
architectures. __ARM_FEATURE_CLZ is not used because its value depends on what 
mode the user is targeting but only the architecture support matters in this 
case. Indeed, the code using CLZ is written in assembler and uses mnemonics 
available both in ARM and Thumb mode so only CLZ availability in one of the 
mode matters.

This change was split out from [PATCH, GCC, ARM 1/7] Fix Thumb-1 only == 
ARMv6-M & Thumb-2 only == ARMv7-M assumptions.

ChangeLog entry is as follows:

*** libgcc/ChangeLog ***

2016-06-16  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * config/arm/lib1funcs.S (HAVE_ARM_CLZ): Define for ARMv6* or later
        and ARMv5t* rather than for a fixed list of architectures.

Looking for code generation change accross a number of combinations of ISAs 
(ARM/Thumb), optimization levels (Os/O2), and architectures (armv4, armv4t, 
armv5, armv5t, armv5te, armv6, armv6j, armv6k, armv6s-m, armv6kz, armv6t2, 
armv6z, armv6zk, armv7, armv7-a, armv7e-m, armv7-m, armv7-r, armv7ve, armv8-a, 
armv8-a+crc, iwmmxt and iwmmxt2) shows that only ARMv5T is impacted (uses CLZ 
now). This is expected because currently HAVE_ARM_CLZ is not defined for this 
architecture while the ARMv7-a/ARMv7-R Architecture Reference Manual [1] 
states that all ARMv5T* architectures have CLZ. ARMv5E should also be impacted 
(not using CLZ anymore) but testing it is difficult since current binutils does 
not support ARMv5E.

[1] Document ARM DDI0406C in http://infocenter.arm.com

Best regards,

Thomas

Comments

Thomas Preudhomme July 5, 2016, 9:30 a.m. UTC | #1 
[Fixed subject to reflect patch]

Ping?

Best regards,

Thomas

On Monday 27 June 2016 17:51:34 Thomas Preudhomme wrote:
> Hi Ramana,
> 
> On Wednesday 01 June 2016 10:00:52 Ramana Radhakrishnan wrote:
> > From here down to ....
> > 
> > > -#if ((__ARM_ARCH__ > 5) && !defined(__ARM_ARCH_6M__)) \
> > > -    || defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) \
> > > -    || defined(__ARM_ARCH_5TEJ__)
> > > -#define HAVE_ARM_CLZ 1
> > > -#endif
> > > -
> > > 
> > >  #ifdef L_clzsi2
> > > 
> > > -#if defined(__ARM_ARCH_6M__)
> > > +#if !__ARM_ARCH_ISA_ARM && __ARM_ARCH_ISA_THUMB == 1
> > > 
> > >  FUNC_START clzsi2
> > >  
> > >         mov     r1, #28
> > >         mov     r3, #1
> > > 
> > > @@ -1544,7 +1538,7 @@ FUNC_START clzsi2
> > > 
> > >         FUNC_END clzsi2
> > >  
> > >  #else
> > >  ARM_FUNC_START clzsi2
> > > 
> > > -# if defined(HAVE_ARM_CLZ)
> > > +# if defined(__ARM_FEATURE_CLZ)
> > > 
> > >         clz     r0, r0
> > >         RET
> > >  
> > >  # else
> > > 
> > > @@ -1568,15 +1562,15 @@ ARM_FUNC_START clzsi2
> > > 
> > >  .align 2
> > >  1:
> > >  .byte 4, 3, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0
> > > 
> > > -# endif /* !HAVE_ARM_CLZ */
> > > +# endif /* !__ARM_FEATURE_CLZ */
> > > 
> > >         FUNC_END clzsi2
> > >  
> > >  #endif
> > >  #endif /* L_clzsi2 */
> > >  
> > >  #ifdef L_clzdi2
> > > 
> > > -#if !defined(HAVE_ARM_CLZ)
> > > +#if !defined(__ARM_FEATURE_CLZ)
> > 
> > here should be it's own little patchlet and can  go in separately.
> 
> The patch in attachment changes the CLZ availability check in libgcc to test
> ISA supported and architecture version rather than encode a specific list
> of architectures. __ARM_FEATURE_CLZ is not used because its value depends
> on what mode the user is targeting but only the architecture support
> matters in this case. Indeed, the code using CLZ is written in assembler
> and uses mnemonics available both in ARM and Thumb mode so only CLZ
> availability in one of the mode matters.
> 
> This change was split out from [PATCH, GCC, ARM 1/7] Fix Thumb-1 only ==
> ARMv6-M & Thumb-2 only == ARMv7-M assumptions.
> 
> ChangeLog entry is as follows:
> 
> *** libgcc/ChangeLog ***
> 
> 2016-06-16  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * config/arm/lib1funcs.S (HAVE_ARM_CLZ): Define for ARMv6* or later
>         and ARMv5t* rather than for a fixed list of architectures.
> 
> Looking for code generation change accross a number of combinations of ISAs
> (ARM/Thumb), optimization levels (Os/O2), and architectures (armv4, armv4t,
> armv5, armv5t, armv5te, armv6, armv6j, armv6k, armv6s-m, armv6kz, armv6t2,
> armv6z, armv6zk, armv7, armv7-a, armv7e-m, armv7-m, armv7-r, armv7ve,
> armv8-a, armv8-a+crc, iwmmxt and iwmmxt2) shows that only ARMv5T is
> impacted (uses CLZ now). This is expected because currently HAVE_ARM_CLZ is
> not defined for this architecture while the ARMv7-a/ARMv7-R Architecture
> Reference Manual [1] states that all ARMv5T* architectures have CLZ. ARMv5E
> should also be impacted (not using CLZ anymore) but testing it is difficult
> since current binutils does not support ARMv5E.
> 
> [1] Document ARM DDI0406C in http://infocenter.arm.com
> 
> Best regards,
> 
> Thomas
Ramana Radhakrishnan July 6, 2016, 4:20 p.m. UTC | #2 
On Mon, Jun 27, 2016 at 5:51 PM, Thomas Preudhomme
<thomas.preudhomme@foss.arm.com> wrote:
> Hi Ramana,
>
> On Wednesday 01 June 2016 10:00:52 Ramana Radhakrishnan wrote:
>>
>> From here down to ....
>>
>> > -#if ((__ARM_ARCH__ > 5) && !defined(__ARM_ARCH_6M__)) \
>> > -    || defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) \
>> > -    || defined(__ARM_ARCH_5TEJ__)
>> > -#define HAVE_ARM_CLZ 1
>> > -#endif
>> > -
>> >
>> >  #ifdef L_clzsi2
>> >
>> > -#if defined(__ARM_ARCH_6M__)
>> > +#if !__ARM_ARCH_ISA_ARM && __ARM_ARCH_ISA_THUMB == 1
>> >
>> >  FUNC_START clzsi2
>> >
>> >         mov     r1, #28
>> >         mov     r3, #1
>> >
>> > @@ -1544,7 +1538,7 @@ FUNC_START clzsi2
>> >
>> >         FUNC_END clzsi2
>> >
>> >  #else
>> >  ARM_FUNC_START clzsi2
>> >
>> > -# if defined(HAVE_ARM_CLZ)
>> > +# if defined(__ARM_FEATURE_CLZ)
>> >
>> >         clz     r0, r0
>> >         RET
>> >
>> >  # else
>> >
>> > @@ -1568,15 +1562,15 @@ ARM_FUNC_START clzsi2
>> >
>> >  .align 2
>> >  1:
>> >  .byte 4, 3, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0
>> >
>> > -# endif /* !HAVE_ARM_CLZ */
>> > +# endif /* !__ARM_FEATURE_CLZ */
>> >
>> >         FUNC_END clzsi2
>> >
>> >  #endif
>> >  #endif /* L_clzsi2 */
>> >
>> >  #ifdef L_clzdi2
>> >
>> > -#if !defined(HAVE_ARM_CLZ)
>> > +#if !defined(__ARM_FEATURE_CLZ)
>>
>> here should be it's own little patchlet and can  go in separately.
>
> The patch in attachment changes the CLZ availability check in libgcc to test
> ISA supported and architecture version rather than encode a specific list of
> architectures. __ARM_FEATURE_CLZ is not used because its value depends on what
> mode the user is targeting but only the architecture support matters in this
> case. Indeed, the code using CLZ is written in assembler and uses mnemonics
> available both in ARM and Thumb mode so only CLZ availability in one of the
> mode matters.
>
> This change was split out from [PATCH, GCC, ARM 1/7] Fix Thumb-1 only ==
> ARMv6-M & Thumb-2 only == ARMv7-M assumptions.
>
> ChangeLog entry is as follows:
>
> *** libgcc/ChangeLog ***
>
> 2016-06-16  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>
>         * config/arm/lib1funcs.S (HAVE_ARM_CLZ): Define for ARMv6* or later
>         and ARMv5t* rather than for a fixed list of architectures.
>
> Looking for code generation change accross a number of combinations of ISAs
> (ARM/Thumb), optimization levels (Os/O2), and architectures (armv4, armv4t,
> armv5, armv5t, armv5te, armv6, armv6j, armv6k, armv6s-m, armv6kz, armv6t2,
> armv6z, armv6zk, armv7, armv7-a, armv7e-m, armv7-m, armv7-r, armv7ve, armv8-a,
> armv8-a+crc, iwmmxt and iwmmxt2) shows that only ARMv5T is impacted (uses CLZ
> now). This is expected because currently HAVE_ARM_CLZ is not defined for this
> architecture while the ARMv7-a/ARMv7-R Architecture Reference Manual [1]
> states that all ARMv5T* architectures have CLZ. ARMv5E should also be impacted
> (not using CLZ anymore) but testing it is difficult since current binutils does
> not support ARMv5E.
>
> [1] Document ARM DDI0406C in http://infocenter.arm.com
>
> Best regards,
>
> Thomas



OK.

Ramana
diff mbox

Patch

diff --git a/libgcc/config/arm/lib1funcs.S b/libgcc/config/arm/lib1funcs.S
index 951dcda1c3bf7f323423a3e2813bdf0501653016..c4f061f8196d243159903cac4eb0291d1bf0b1ad 100644
--- a/libgcc/config/arm/lib1funcs.S
+++ b/libgcc/config/arm/lib1funcs.S
@@ -1512,9 +1512,10 @@  LSYM(Lover12):
 
 #endif /* __symbian__ */
 
-#if ((__ARM_ARCH__ > 5) && !defined(__ARM_ARCH_6M__)) \
-    || defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) \
-    || defined(__ARM_ARCH_5TEJ__)
+#if (__ARM_ARCH_ISA_THUMB == 2	\
+     || (__ARM_ARCH_ISA_ARM	\
+	 && (__ARM_ARCH__ > 5	\
+	     || (__ARM_ARCH__ == 5 && __ARM_ARCH_ISA_THUMB))))
 #define HAVE_ARM_CLZ 1
 #endif