This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


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.


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,
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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]