[PATCH, AARCH64] make stdarg functions work with +nofp

[PATCH, AARCH64] make stdarg functions work with +nofp

[Jim Wilson]

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

The compiler currently ICEs when compiling a stdarg function with
+nofp, as reported in PR 66258.

The aarch64.md file disables FP instructions using TARGET_FLOAT, which
supports both -mgeneral-regs-only and +nofp.  But there is code in
aarch64.c that checks TARGET_GENERAL_REGS_ONLY.  This results in FP
instructions when using +nofp,  The aarch64.c code needs to use
TARGET_FLOAT instead like the md file already does.

I can't meaningfully test this with a bootstrap, since the patch has
no effect unless I bootstrap with +nofp, and that will fail as gcc
contains floating point code.

The testsuite already has multiple stdarg tests, so there is no need
for another one.

I tested this by verifying I get the same results for some simple
testcasess with and without the patch, with and without using
-mgeneral-regs-only and -mcpu=cortex-a53+nofp.

[-- Attachment #2: a64-nofp-stdarg.patch --]
[-- Type: text/x-patch, Size: 2266 bytes --]

2015-05-22  Jim Wilson  <jim.wilson@linaro.org>

	PR target/66258
	* config/aarch64/aarch64.c (aarch64_function_value_regno_p): Change
	!TARGET_GENERAL_REGS_ONLY to TARGET_FLOAT.
	(aarch64_secondary_reload): Likewise
	(aarch64_expand_builtin_va_start): Change TARGET_GENERAL_REGS_ONLY
	to !TARGET_FLOAT.
	(aarch64_gimplify_va_arg_expr, aarch64_setup_incoming_varargs):
	Likewise.

Index: config/aarch64/aarch64.c
===================================================================
--- config/aarch64/aarch64.c	(revision 223590)
+++ config/aarch64/aarch64.c	(working copy)
@@ -1666,7 +1666,7 @@ aarch64_function_value_regno_p (const un
   /* Up to four fp/simd registers can return a function value, e.g. a
      homogeneous floating-point aggregate having four members.  */
   if (regno >= V0_REGNUM && regno < V0_REGNUM + HA_MAX_NUM_FLDS)
-    return !TARGET_GENERAL_REGS_ONLY;
+    return TARGET_FLOAT;
 
   return false;
 }
@@ -4783,7 +4783,7 @@ aarch64_secondary_reload (bool in_p ATTR
   /* A TFmode or TImode memory access should be handled via an FP_REGS
      because AArch64 has richer addressing modes for LDR/STR instructions
      than LDP/STP instructions.  */
-  if (!TARGET_GENERAL_REGS_ONLY && rclass == GENERAL_REGS
+  if (TARGET_FLOAT && rclass == GENERAL_REGS
       && GET_MODE_SIZE (mode) == 16 && MEM_P (x))
     return FP_REGS;
 
@@ -7571,7 +7571,7 @@ aarch64_expand_builtin_va_start (tree va
   vr_save_area_size
     = (NUM_FP_ARG_REGS - cum->aapcs_nvrn) * UNITS_PER_VREG;
 
-  if (TARGET_GENERAL_REGS_ONLY)
+  if (!TARGET_FLOAT)
     {
       if (cum->aapcs_nvrn > 0)
 	sorry ("%qs and floating point or vector arguments",
@@ -7681,7 +7681,7 @@ aarch64_gimplify_va_arg_expr (tree valis
 					       &is_ha))
     {
       /* TYPE passed in fp/simd registers.  */
-      if (TARGET_GENERAL_REGS_ONLY)
+      if (!TARGET_FLOAT)
 	sorry ("%qs and floating point or vector arguments",
 	       "-mgeneral-regs-only");
 
@@ -7918,7 +7918,7 @@ aarch64_setup_incoming_varargs (cumulati
   gr_saved = NUM_ARG_REGS - local_cum.aapcs_ncrn;
   vr_saved = NUM_FP_ARG_REGS - local_cum.aapcs_nvrn;
 
-  if (TARGET_GENERAL_REGS_ONLY)
+  if (!TARGET_FLOAT)
     {
       if (local_cum.aapcs_nvrn > 0)
 	sorry ("%qs and floating point or vector arguments",

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[James Greenhalgh]

On Sat, May 23, 2015 at 12:24:00AM +0100, Jim Wilson wrote:
> The compiler currently ICEs when compiling a stdarg function with
> +nofp, as reported in PR 66258.
> 
> The aarch64.md file disables FP instructions using TARGET_FLOAT, which
> supports both -mgeneral-regs-only and +nofp.  But there is code in
> aarch64.c that checks TARGET_GENERAL_REGS_ONLY.  This results in FP
> instructions when using +nofp,  The aarch64.c code needs to use
> TARGET_FLOAT instead like the md file already does.
> 
> I can't meaningfully test this with a bootstrap, since the patch has
> no effect unless I bootstrap with +nofp, and that will fail as gcc
> contains floating point code.
> 
> The testsuite already has multiple stdarg tests, so there is no need
> for another one.
> 
> I tested this by verifying I get the same results for some simple
> testcasess with and without the patch, with and without using
> -mgeneral-regs-only and -mcpu=cortex-a53+nofp.

This patch doesn't quite look right to me. The cases you change seem
like they should be (TARGET_FLOAT || TARGET_SIMD), rather than just
TARGET_FLOAT. In an armv8-a+nofp environment, you still have access to the
SIMD registers and instructions (reading between the lines on the bug
report, this is almost certainly not what is intended in Grub!).

Digging a bit deeper in to the ICE in PR66258, it seems to me that
the problematic pattern is "*movti_aarch64":

  (define_insn "*movti_aarch64"
    [(set (match_operand:TI 0
	   "nonimmediate_operand"  "=r, *w,r ,*w,r  ,Ump,Ump,*w,m")
	  (match_operand:TI 1
	   "aarch64_movti_operand" " rn,r ,*w,*w,Ump,r  ,Z  , m,*w"))]
    "(register_operand (operands[0], TImode)
      || aarch64_reg_or_zero (operands[1], TImode))"
    "@
     #
     #
     #
     orr\\t%0.16b, %1.16b, %1.16b
     ldp\\t%0, %H0, %1
     stp\\t%1, %H1, %0
     stp\\txzr, xzr, %0
     ldr\\t%q0, %1
     str\\t%q1, %0"
    [(set_attr "type" "multiple,f_mcr,f_mrc,neon_logic_q, \
	             load2,store2,store2,f_loadd,f_stored")
     (set_attr "length" "8,8,8,4,4,4,4,4,4")
     (set_attr "simd" "*,*,*,yes,*,*,*,*,*")
     (set_attr "fp" "*,*,*,*,*,*,*,yes,yes")]
  )

Note that the split alternatives are going to unconditionally create
and emit insns which require TARGET_FLOAT, but the fp attribute is
not set on those alternatives. Many of the TI mode split patterns
could be expressed as a umov from vector registers to general purpose
registers for a TARGET_SIMD target.

Have you investigated this approach at all?

Thanks,
James

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[Kyrill Tkachov]

Hi James, Jim,

On 02/06/15 10:42, James Greenhalgh wrote:
> On Sat, May 23, 2015 at 12:24:00AM +0100, Jim Wilson wrote:
>> The compiler currently ICEs when compiling a stdarg function with
>> +nofp, as reported in PR 66258.
>>
>> The aarch64.md file disables FP instructions using TARGET_FLOAT, which
>> supports both -mgeneral-regs-only and +nofp.  But there is code in
>> aarch64.c that checks TARGET_GENERAL_REGS_ONLY.  This results in FP
>> instructions when using +nofp,  The aarch64.c code needs to use
>> TARGET_FLOAT instead like the md file already does.
>>
>> I can't meaningfully test this with a bootstrap, since the patch has
>> no effect unless I bootstrap with +nofp, and that will fail as gcc
>> contains floating point code.
>>
>> The testsuite already has multiple stdarg tests, so there is no need
>> for another one.
>>
>> I tested this by verifying I get the same results for some simple
>> testcasess with and without the patch, with and without using
>> -mgeneral-regs-only and -mcpu=cortex-a53+nofp.
> This patch doesn't quite look right to me. The cases you change seem
> like they should be (TARGET_FLOAT || TARGET_SIMD), rather than just
> TARGET_FLOAT. In an armv8-a+nofp environment, you still have access to the
> SIMD registers and instructions (reading between the lines on the bug
> report, this is almost certainly not what is intended in Grub!).

I don't think that's quite right. TARGET_SIMD *always* implies TARGET_FP as it is a superset of that functionality.

For the precise relations of them look in aarch64-option-extensions.def.
Turning off fp with +nofp (or -mgeneral-regs-only) always turns off simd while turning off simd
with +nosimd doesn't turn off fp.

Cheers,

Kyrill

>
> Digging a bit deeper in to the ICE in PR66258, it seems to me that
> the problematic pattern is "*movti_aarch64":
>
>    (define_insn "*movti_aarch64"
>      [(set (match_operand:TI 0
> 	   "nonimmediate_operand"  "=r, *w,r ,*w,r  ,Ump,Ump,*w,m")
> 	  (match_operand:TI 1
> 	   "aarch64_movti_operand" " rn,r ,*w,*w,Ump,r  ,Z  , m,*w"))]
>      "(register_operand (operands[0], TImode)
>        || aarch64_reg_or_zero (operands[1], TImode))"
>      "@
>       #
>       #
>       #
>       orr\\t%0.16b, %1.16b, %1.16b
>       ldp\\t%0, %H0, %1
>       stp\\t%1, %H1, %0
>       stp\\txzr, xzr, %0
>       ldr\\t%q0, %1
>       str\\t%q1, %0"
>      [(set_attr "type" "multiple,f_mcr,f_mrc,neon_logic_q, \
> 	             load2,store2,store2,f_loadd,f_stored")
>       (set_attr "length" "8,8,8,4,4,4,4,4,4")
>       (set_attr "simd" "*,*,*,yes,*,*,*,*,*")
>       (set_attr "fp" "*,*,*,*,*,*,*,yes,yes")]
>    )
>
> Note that the split alternatives are going to unconditionally create
> and emit insns which require TARGET_FLOAT, but the fp attribute is
> not set on those alternatives. Many of the TI mode split patterns
> could be expressed as a umov from vector registers to general purpose
> registers for a TARGET_SIMD target.
>
> Have you investigated this approach at all?
>
> Thanks,
> James
>

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[James Greenhalgh]

On Tue, Jun 02, 2015 at 11:38:29AM +0100, Kyrill Tkachov wrote:
> Hi James, Jim,
> 
> On 02/06/15 10:42, James Greenhalgh wrote:
> > On Sat, May 23, 2015 at 12:24:00AM +0100, Jim Wilson wrote:
> >> The compiler currently ICEs when compiling a stdarg function with
> >> +nofp, as reported in PR 66258.
> >>
> >> The aarch64.md file disables FP instructions using TARGET_FLOAT, which
> >> supports both -mgeneral-regs-only and +nofp.  But there is code in
> >> aarch64.c that checks TARGET_GENERAL_REGS_ONLY.  This results in FP
> >> instructions when using +nofp,  The aarch64.c code needs to use
> >> TARGET_FLOAT instead like the md file already does.
> >>
> >> I can't meaningfully test this with a bootstrap, since the patch has
> >> no effect unless I bootstrap with +nofp, and that will fail as gcc
> >> contains floating point code.
> >>
> >> The testsuite already has multiple stdarg tests, so there is no need
> >> for another one.
> >>
> >> I tested this by verifying I get the same results for some simple
> >> testcasess with and without the patch, with and without using
> >> -mgeneral-regs-only and -mcpu=cortex-a53+nofp.
> > This patch doesn't quite look right to me. The cases you change seem
> > like they should be (TARGET_FLOAT || TARGET_SIMD), rather than just
> > TARGET_FLOAT. In an armv8-a+nofp environment, you still have access to the
> > SIMD registers and instructions (reading between the lines on the bug
> > report, this is almost certainly not what is intended in Grub!).
> 
> I don't think that's quite right. TARGET_SIMD *always* implies TARGET_FP as
> it is a superset of that functionality.
> 
> For the precise relations of them look in aarch64-option-extensions.def.
> Turning off fp with +nofp (or -mgeneral-regs-only) always turns off simd
> while turning off simd with +nosimd doesn't turn off fp.

Right, understood. I had incorrectly thought we had kept them as fully
distinct options to disable parts of the ARMv8-A instruction set.

In which case, Jim, your patch is OK. Sorry for my initial confusion.

I think I saw a patch kicking around internally to improve the
documentation in this area, Alan - was that yours?

Thanks,
James
 

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[Jim Wilson]

On Tue, Jun 2, 2015 at 3:45 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> On Tue, Jun 02, 2015 at 11:38:29AM +0100, Kyrill Tkachov wrote:
>> Hi James, Jim,
>>
>> On 02/06/15 10:42, James Greenhalgh wrote:
>> > On Sat, May 23, 2015 at 12:24:00AM +0100, Jim Wilson wrote:
>> >> The compiler currently ICEs when compiling a stdarg function with
>> >> +nofp, as reported in PR 66258.

I'd like approval to add this patch to the gcc-5 release branch.  I
got two requests for this in the PR as currently grub won't build with
gcc-5.1.  I tested the patch on the gcc-5-release branch with a
default languages bootstrap and make check on an APM box running
Ubuntu.  I also verified that the patch fixes my testcase.

Jim

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[James Greenhalgh]

On Tue, Jun 09, 2015 at 03:18:05AM +0100, Jim Wilson wrote:
> On Tue, Jun 2, 2015 at 3:45 AM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
> > On Tue, Jun 02, 2015 at 11:38:29AM +0100, Kyrill Tkachov wrote:
> >> Hi James, Jim,
> >>
> >> On 02/06/15 10:42, James Greenhalgh wrote:
> >> > On Sat, May 23, 2015 at 12:24:00AM +0100, Jim Wilson wrote:
> >> >> The compiler currently ICEs when compiling a stdarg function with
> >> >> +nofp, as reported in PR 66258.
> 
> I'd like approval to add this patch to the gcc-5 release branch.  I
> got two requests for this in the PR as currently grub won't build with
> gcc-5.1.  I tested the patch on the gcc-5-release branch with a
> default languages bootstrap and make check on an APM box running
> Ubuntu.  I also verified that the patch fixes my testcase.

I'm happy for this to be backported.

I think Grub probably wants to change if they want to be safe, from
what I've read it looks like they are hoping to use something like a
standard printf without touching the FP registers, which is suspect...

Thanks,
James

Re: [PATCH, AARCH64] make stdarg functions work with +nofp

[Jim Wilson]

On Tue, Jun 16, 2015 at 1:46 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> I'm happy for this to be backported.

Thanks.  Applied.

> I think Grub probably wants to change if they want to be safe, from
> what I've read it looks like they are hoping to use something like a
> standard printf without touching the FP registers, which is suspect...

Grub has its own printf code, as it can't use glibc, and the
grub_printf function doesn't support FP format codes.  So this should
not be a problem for grub.

FYI In the git tree, grub changed from using +nofp to
-mgeneral-regs-only on June 2, because the kernel uses
-mgeneral-regs-only, and because +nofp was broken in gcc-5-branch.
But we still needed the patch backported for people who could not or
didn't want to upgrade grub.

Jim