This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][ARM] Use __ARM_ARCH instead of __ARM_ARCH__
On Thu, 21 Jun 2018 at 10:00, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
>
> On 21/06/18 07:59, Christophe Lyon wrote:
> > 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.
>
> yes, that looks good now.
>
> > 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)
>
> I see your point. It could indeed be cleaner if the code removal hunk was
> put in a separate patch. A bugzilla entry about the dead code to be removed would
> be appreciated, I can take care of that then.
>
OK, I've filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86264
> > Similarly there is code to define __ARM_ARCH in libffi/src/arm/sysv.S.
>
> I believe libffi is its own separate project that we import in GCC, so it may want
> to support compiling with older GCC versions. I'd need to double-check that.
>
Indeed, that worried me too.
> > So it seems further cleanup would be needed.
>
> Indeed. This patch is ok with the __ARM_ARCH < 4 path removals separated into
> their own patch.
>
Thanks, committed as r261840 and r261841.
> Thanks,
> Kyrill
>
> >
> > 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
> >>
>