On Thu, 21 Jun 2018 at 10:00, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote: > > > On 21/06/18 07:59, Christophe Lyon wrote: > > On Tue, 19 Jun 2018 at 10:50, Kyrill Tkachov > > <kyrylo.tkac...@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.earns...@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.l...@linaro.org> > >>>>> > >>>>> * config/arm/arm-config.h (__ARM_ARCH__): Remove definitions, use > >>>>> __ARM_ARCH instead. > >>>>> > >>>>> libgcc/ChangeLog: > >>>>> > >>>>> 2018-06-15 Christophe Lyon <christophe.l...@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 > >> >