Message ID | CABXYE2V+SwMbT5EsYM6hD0ENjwAhsZKAPTLvDuLxCNX9GAhLvg@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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 >
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
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
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
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
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",