[PATCH][RFC][libatomic] Override -mcpu option for arm linux ifunc targets

[PATCH][RFC][libatomic] Override -mcpu option for arm linux ifunc targets

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

Hi all,

There is a slight issue with the libatomic Makefile for arm linux ifunc targets. 
It adds an explicity -march=armv7-a option to the command line to enable 
building the exclusive instruction variants in libatomic. However, if the 
multilib machinery tries to add an -mcpu option that conflicts with the -march 
one (such as -mcpu=cortex-a15) gcc will give a warning about incompatible -march 
and -mcpu options, causing the -Werror build to fail.

A workaround here is to override the -mcpu option as well as the -march one. 
This patch does that by adding an EXTRA_OVERRIDE variable and setting it to 
-mcpu=cortex-a9 under the same conditions as when -march=armv7-a is selected, so 
that it's added only when -march=armv70a is added.

Can someone see a better way of achieving this?

If this is acceptable, ok to commit?

Build and test arm-none-linux-gnueabi with --enable-gnu-indirect-function
Bootstrap on x86 with --enable-gnu-indirect-function

Thanks,
Kyrill

2014-02-03  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * Makefile.in: Override -mcpu option when building arm
     linux ifunc targets.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libatomic-ifunc.patch --]
[-- Type: text/x-patch; name=libatomic-ifunc.patch, Size: 1420 bytes --]

diff --git a/libatomic/Makefile.in b/libatomic/Makefile.in
index 46e60c9..b51910a 100644
--- a/libatomic/Makefile.in
+++ b/libatomic/Makefile.in
@@ -297,7 +297,7 @@ PAT_BASE = $(word 1,$(PAT_SPLIT))
 PAT_N = $(word 2,$(PAT_SPLIT))
 PAT_S = $(word 3,$(PAT_SPLIT))
 IFUNC_DEF = -DIFUNC_ALT=$(PAT_S)
-IFUNC_OPT = $(word $(PAT_S),$(IFUNC_OPTIONS))
+IFUNC_OPT = $(word $(PAT_S),$(IFUNC_OPTIONS)) $(word $(PAT_S),$(EXTRA_OVERRIDE))
 M_DEPS = -MT $@ -MD -MP -MF $(DEPDIR)/$(@F).Ppo
 M_SIZE = -DN=$(PAT_N)
 M_IFUNC = $(if $(PAT_S),$(IFUNC_DEF) $(IFUNC_OPT))
@@ -313,7 +313,13 @@ M_SRC = $(firstword $(filter %/$(M_FILE), $(all_c_files)))
 libatomic_la_LIBADD = $(foreach s,$(SIZES),$(addsuffix \
 	_$(s)_.lo,$(SIZEOBJS))) $(am__append_1) $(am__append_2) \
 	$(am__append_3)
+# For ARM, the -march option by itself conflicts with any -mcpu option that
+# we might end up passing to the build, causing an error.
+# Therefore we override the -mcpu option as well.
+# This shouldn't affect tuning much because the affected code is mostly
+# in inline assembly anyway.
 @ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=armv7-a -DHAVE_KERNEL64
+@ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@EXTRA_OVERRIDE = -mcpu=cortex-a9
 @ARCH_I386_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=i586
 @ARCH_X86_64_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -mcx16
 libatomic_convenience_la_SOURCES = $(libatomic_la_SOURCES)

Re: [PATCH][RFC][libatomic] Override -mcpu option for arm linux ifunc targets

[Kyrill Tkachov]

Ping?
http://gcc.gnu.org/ml/gcc-patches/2014-02/msg00069.html

On 03/02/14 11:50, Kyrill Tkachov wrote:
> Hi all,
>
> There is a slight issue with the libatomic Makefile for arm linux ifunc targets.
> It adds an explicity -march=armv7-a option to the command line to enable
> building the exclusive instruction variants in libatomic. However, if the
> multilib machinery tries to add an -mcpu option that conflicts with the -march
> one (such as -mcpu=cortex-a15) gcc will give a warning about incompatible -march
> and -mcpu options, causing the -Werror build to fail.
>
> A workaround here is to override the -mcpu option as well as the -march one.
> This patch does that by adding an EXTRA_OVERRIDE variable and setting it to
> -mcpu=cortex-a9 under the same conditions as when -march=armv7-a is selected, so
> that it's added only when -march=armv70a is added.
>
> Can someone see a better way of achieving this?
>
> If this is acceptable, ok to commit?
>
> Build and test arm-none-linux-gnueabi with --enable-gnu-indirect-function
> Bootstrap on x86 with --enable-gnu-indirect-function
>
> Thanks,
> Kyrill
>
> 2014-02-03  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       * Makefile.in: Override -mcpu option when building arm
>       linux ifunc targets.

Re: [PATCH][RFC][libatomic] Override -mcpu option for arm linux ifunc targets

[Richard Henderson]

On 02/03/2014 03:50 AM, Kyrill Tkachov wrote:
> +# For ARM, the -march option by itself conflicts with any -mcpu option that
> +# we might end up passing to the build, causing an error.
> +# Therefore we override the -mcpu option as well.
> +# This shouldn't affect tuning much because the affected code is mostly
> +# in inline assembly anyway.
>  @ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=armv7-a -DHAVE_KERNEL64
> +@ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@EXTRA_OVERRIDE = -mcpu=cortex-a9

Why would you want to split these across two different variables?  It's easier
to just add the -march and -mcpu to the same IFUNC_OPTIONS variable.

Why the choice of cortext-a9, as opposed to any of the other v7-a
possibilities?  If we're going to force anything, perhaps generic-armv7-a is
more appropriate?


r~

Re: [PATCH][RFC][libatomic] Override -mcpu option for arm linux ifunc targets

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

On 13/02/14 18:23, Richard Henderson wrote:
> On 02/03/2014 03:50 AM, Kyrill Tkachov wrote:
>> +# For ARM, the -march option by itself conflicts with any -mcpu option that
>> +# we might end up passing to the build, causing an error.
>> +# Therefore we override the -mcpu option as well.
>> +# This shouldn't affect tuning much because the affected code is mostly
>> +# in inline assembly anyway.
>>   @ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=armv7-a -DHAVE_KERNEL64
>> +@ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@EXTRA_OVERRIDE = -mcpu=cortex-a9
> Why would you want to split these across two different variables?  It's easier
> to just add the -march and -mcpu to the same IFUNC_OPTIONS variable.

Because IFUNC_OPTIONS is not used as is. It goes through some kind of machinery 
with the PAT_N, PAT_S, PAT_BASe, PAT_SPLIT above that is used to extract just 
one of the options from IFUNC_OPTIONS and adding a new entry there ends up 
messing up the expected order.
If there's an easier way to specify the extra option I'll be happy to use it, 
but I couldn't find a way of doing it without rewriting a whole lot of Makefile 
magic.

>
> Why the choice of cortext-a9, as opposed to any of the other v7-a
> possibilities?  If we're going to force anything, perhaps generic-armv7-a is
> more appropriate?

Hmmm, you're right. Might as well make it explicit that we want armv7-a.

How about this then?

Thanks for the review,
Kyrill

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libatomic-ifunc.patch --]
[-- Type: text/x-patch; name=libatomic-ifunc.patch, Size: 1426 bytes --]

diff --git a/libatomic/Makefile.in b/libatomic/Makefile.in
index 46e60c9..b51910a 100644
--- a/libatomic/Makefile.in
+++ b/libatomic/Makefile.in
@@ -297,7 +297,7 @@ PAT_BASE = $(word 1,$(PAT_SPLIT))
 PAT_N = $(word 2,$(PAT_SPLIT))
 PAT_S = $(word 3,$(PAT_SPLIT))
 IFUNC_DEF = -DIFUNC_ALT=$(PAT_S)
-IFUNC_OPT = $(word $(PAT_S),$(IFUNC_OPTIONS))
+IFUNC_OPT = $(word $(PAT_S),$(IFUNC_OPTIONS)) $(word $(PAT_S),$(EXTRA_OVERRIDE))
 M_DEPS = -MT $@ -MD -MP -MF $(DEPDIR)/$(@F).Ppo
 M_SIZE = -DN=$(PAT_N)
 M_IFUNC = $(if $(PAT_S),$(IFUNC_DEF) $(IFUNC_OPT))
@@ -313,7 +313,13 @@ M_SRC = $(firstword $(filter %/$(M_FILE), $(all_c_files)))
 libatomic_la_LIBADD = $(foreach s,$(SIZES),$(addsuffix \
 	_$(s)_.lo,$(SIZEOBJS))) $(am__append_1) $(am__append_2) \
 	$(am__append_3)
+# For ARM, the -march option by itself conflicts with any -mcpu option that
+# we might end up passing to the build, causing an error.
+# Therefore we override the -mcpu option as well.
+# This shouldn't affect tuning much because the affected code is mostly
+# in inline assembly anyway.
 @ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=armv7-a -DHAVE_KERNEL64
+@ARCH_ARM_LINUX_TRUE@@HAVE_IFUNC_TRUE@EXTRA_OVERRIDE = -mcpu=generic-armv7-a
 @ARCH_I386_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -march=i586
 @ARCH_X86_64_TRUE@@HAVE_IFUNC_TRUE@IFUNC_OPTIONS = -mcx16
 libatomic_convenience_la_SOURCES = $(libatomic_la_SOURCES)