Re: [PATCH][ARM] Use __ARM_ARCH instead of __ARM_ARCH__

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On Tue, 19 Jun 2018 at 10:50, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> Hi Christophe,
>
> On 17/06/18 21:23, Christophe Lyon wrote:
> > On Fri, 15 Jun 2018 at 17:22, Richard Earnshaw (lists)
> > <Richard.Earnshaw@arm.com> wrote:
> > >
> > > On 15/06/18 15:30, Christophe Lyon wrote:
> > > > Hello,
> > > >
> > > > As suggested in [1], the attached patch removes all definitions and
> > > > uses of __ARM_ARCH__ and uses __ARM_ARCH instead. The later is indeed
> > > > defined by the preprocessor to the appropriate value.
> > > >
> > > > I ran make check on arm-none-eabi (with A-profile multilib),
> > > > arm-none-linux-gnueabi, arm-none-linux-gnueabihf (with cortex-a9, a15,
> > > > a5, a57 and armtdmi as --with-cpu), armeb-none-linux-gnueabihf and
> > > > armv8l-linux-gnueabihf, and noticed no regression.
> > > >
> > > > OK for trunk?
> > > >
> > > > Thanks,
> > > >
> > > > Christophe
> > > >
> > > > [1] https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00445.html
> > > >
> > > >
> > > > ARM_ARCH.chlog.txt
> > > >
> > > >
> > > > libatomic/ChangeLog:
> > > >
> > > > 2018-06-15  Christophe Lyon <christophe.lyon@linaro.org>
> > > >
> > > >       * config/arm/arm-config.h (__ARM_ARCH__): Remove definitions, use
> > > >       __ARM_ARCH instead.
> > > >
> > > > libgcc/ChangeLog:
> > > >
> > > > 2018-06-15  Christophe Lyon <christophe.lyon@linaro.org>
> > > >
> > > >       * config/arm/lib1funcs.S (__ARM_ARCH__): Remove definitions, use
> > > >       __ARM_ARCH instead.
> > > >       * config/arm/ieee754-df.S: Use __ARM_ARCH instead of __ARM_ARCH__.
> > > >       * config/arm/ieee754-sf.S: Likewise.
> > > >       * config/arm/libunwind.S: Likewise.
> > > >
> > > >
> > > > ARM_ARCH.patch.txt
> > > >
> > >
> > > Thanks, this is a useful start.  We can, however, go further.  ACLE
> > > defines a number of 'feature' pre-defines and we can use those to void
> > > direct tests of the architecture version directly.  For example,
> > > __ARM_FEATURE_LDREX could directly replace having to calculate
> > > HAVE_STREX and HAVE_STREXBHD.
> > >
> > Hi,
> >
> > Here is an updated patch using __ARM_FEATURE_LDREX.
> > I didn't find other opportunities to use ACLE pre-defines, did I miss any?
> >
>
> Thanks for doing this. I think we can catch a few more...
>
OK, I didn't grep accurately enough it seems.

Here is a new version hopefully addressing your comments.

However, I'm not sure whether replacing uses of __ARM_ARCH__ and
removing support for arches < 4 should be in the same patch: this goes
beyond my original intent, and I've noticed probable dead code in
include/longlong.h (support for umul_ppmm on arm v2 and v3)

Similarly there is code to define __ARM_ARCH in libffi/src/arm/sysv.S.

So it seems further cleanup would be needed.

Christophe


> diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S
> index 570e5f6..7c5260e 100644
> --- a/libgcc/config/arm/ieee754-df.S
> +++ b/libgcc/config/arm/ieee754-df.S
> @@ -245,7 +245,7 @@ LSYM(Lad_a):
>         @ No rounding necessary since ip will always be 0 at this point.
>   LSYM(Lad_l):
>
> -#if __ARM_ARCH__ < 5
> +#if __ARM_ARCH < 5
>
> This path exists to handle the case when the CLZ instruction is not available (the #else path uses CLZ).
> So we can change this to #ifndef __ARM_FEATURE_CLZ
>
>
>         teq     xh, #0
>         movne   r3, #20
> @@ -656,7 +656,7 @@ ARM_FUNC_ALIAS aeabi_dmul muldf3
>         orr     yh, yh, #0x00100000
>         beq     LSYM(Lml_1)
>
> -#if __ARM_ARCH__ < 4
> +#if __ARM_ARCH < 4
>
> We can delete this whole path as we no longer support anything older than 4
>
> diff --git a/libgcc/config/arm/ieee754-sf.S b/libgcc/config/arm/ieee754-sf.S
> index dac3e2e..00a8d9c 100644
> --- a/libgcc/config/arm/ieee754-sf.S
> +++ b/libgcc/config/arm/ieee754-sf.S
> @@ -175,7 +175,7 @@ LSYM(Lad_a):
>         @ No rounding necessary since r1 will always be 0 at this point.
>   LSYM(Lad_l):
>
> -#if __ARM_ARCH__ < 5
> +#if __ARM_ARCH < 5
>
>         movs    ip, r0, lsr #12
>         moveq   r0, r0, lsl #12
> @@ -370,7 +370,7 @@ ARM_FUNC_ALIAS aeabi_l2f floatdisf
>         subeq   r3, r3, #(32 << 23)
>   2:    sub     r3, r3, #(1 << 23)
>
> -#if __ARM_ARCH__ < 5
> +#if __ARM_ARCH < 5
>
>         mov     r2, #23
>         cmp     ip, #(1 << 16)
>
> Similar comment on checking __ARM_FEATURE_CLZ in the above two checks.
>
>
>   @@ -460,7 +460,7 @@ LSYM(Lml_x):
>         orr     r0, r3, r0, lsr #5
>         orr     r1, r3, r1, lsr #5
>
> -#if __ARM_ARCH__ < 4
> +#if __ARM_ARCH < 4
>
>         @ Put sign bit in r3, which will be restored into r0 later.
>         and     r3, ip, #0x80000000
>
>
> Likewise on deleting the < 4 path.
>
> diff --git a/libgcc/config/arm/lib1funcs.S b/libgcc/config/arm/lib1funcs.S
> index 04c1b77..264d54a 100644
> --- a/libgcc/config/arm/lib1funcs.S
> +++ b/libgcc/config/arm/lib1funcs.S
> @@ -74,49 +74,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>
> <snip>
>
> -#if __ARM_ARCH__ >= 5 && ! defined (__OPTIMIZE_SIZE__)
> +#if __ARM_ARCH >= 5 && ! defined (__OPTIMIZE_SIZE__)
>
>
> Likewise I believe this check should be for __ARM_FEATURE_CLZ, with the rest of the #elses
> updated accordingly.
>
> <snip>
>
> @@ -1701,8 +1658,8 @@ LSYM(Lover12):
>
>   #if (__ARM_ARCH_ISA_THUMB == 2        \
>        || (__ARM_ARCH_ISA_ARM   \
> -        && (__ARM_ARCH__ > 5   \
> -            || (__ARM_ARCH__ == 5 && __ARM_ARCH_ISA_THUMB))))
> +        && (__ARM_ARCH > 5     \
> +            || (__ARM_ARCH == 5 && __ARM_ARCH_ISA_THUMB))))
>   #define HAVE_ARM_CLZ 1
>   #endif
>
> This HAVE_ARM_CLZ should now be redundant as we can update its uses to __ARM_FEATURE_CLZ
>
> Thanks,
> Kyrill
>

Attachment: ARM_ARCH-v5.chlog.txt
Description: Text document

Attachment: ARM_ARCH-v5.patch.txt
Description: Text document