| Message ID | ZFzOQqRuXWoRXHsG@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Fix ICE due to infinite splitting [PR109800] | expand |
Hi Alex, > -----Original Message----- > From: Alex Coplan <Alex.Coplan@arm.com> > Sent: Thursday, May 11, 2023 12:15 PM > To: gcc-patches@gcc.gnu.org > Cc: nickc@redhat.com; Richard Earnshaw <Richard.Earnshaw@arm.com>; > Ramana Radhakrishnan <ramana.gcc@gmail.com>; Kyrylo Tkachov > <Kyrylo.Tkachov@arm.com> > Subject: [PATCH] arm: Fix ICE due to infinite splitting [PR109800] > > Hi, > > In r11-966-g9a182ef9ee011935d827ab5c6c9a7cd8e22257d8 we introduce a > simplification to emit_move_insn that attempts to simplify moves of the > form: > > (set (subreg:M1 (reg:M2 ...)) (constant C)) > > where M1 and M2 are of equal mode size. That is problematic for the splitter > vfp.md:no_literal_pool_df_immediate in the arm backend, which tries to pun > an > lvalue DFmode pseudo into DImode and assign a constant to it with > emit_move_insn, as the new transformation simply undoes this, and we end > up > splitting indefinitely. > > This patch changes things around in the arm backend so that we use a > DImode temporary (instead of DFmode) and first load the DImode constant > into the pseudo, and then pun the pseudo into DFmode as an rvalue in a > reg -> reg move. I believe this should be semantically equivalent but > avoids the pathalogical behaviour seen in the PR. > > Bootstrapped/regtested on arm-linux-gnueabihf, regtested on > arm-none-eabi and armeb-none-eabi. > > OK for trunk and backports? Ok but the testcase... > > Thanks, > Alex > > gcc/ChangeLog: > > PR target/109800 > * config/arm/arm.md (movdf): Generate temporary pseudo in > DImode > instead of DFmode. > * config/arm/vfp.md (no_literal_pool_df_immediate): Rather than > punning an > lvalue DFmode pseudo into DImode, use a DImode pseudo and pun it > into > DFmode as an rvalue. > > gcc/testsuite/ChangeLog: > > PR target/109800 > * gcc.target/arm/pr109800.c: New test. diff --git a/gcc/testsuite/gcc.target/arm/pr109800.c b/gcc/testsuite/gcc.target/arm/pr109800.c new file mode 100644 index 00000000000..71d1ede13dd --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr109800.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mbig-endian -mpure-code" } */ +double f() { return 5.0; } ... The arm testsuite options are kinda hard to get right with all the effective targets and multilibs and such hardcoded abi and march options tend to break in some target. I suggest you put this testcase in gcc.target/arm/pure-code and add a dg-skip-if to skip the test if the multilib options specify a different float-abi. Thanks, Kyrill
Hi Kyrill, On 23/05/2023 11:14, Kyrylo Tkachov wrote: > Hi Alex, > diff --git a/gcc/testsuite/gcc.target/arm/pr109800.c b/gcc/testsuite/gcc.target/arm/pr109800.c > new file mode 100644 > index 00000000000..71d1ede13dd > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/pr109800.c > @@ -0,0 +1,3 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mbig-endian -mpure-code" } */ > +double f() { return 5.0; } > > ... The arm testsuite options are kinda hard to get right with all the effective targets and multilibs and such hardcoded abi and march options tend to break in some target. > I suggest you put this testcase in gcc.target/arm/pure-code and add a dg-skip-if to skip the test if the multilib options specify a different float-abi. How about this instead: diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c new file mode 100644 index 00000000000..d797b790232 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c @@ -0,0 +1,4 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_hard_ok } */ +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mbig-endian -mpure-code" } */ +double f() { return 5.0; } Full v2 patch attached. Thanks, Alex diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index cbfc4543531..40c4d848238 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -7555,7 +7555,7 @@ (define_expand "movdf" && !arm_const_double_rtx (operands[1]) && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))) { - rtx clobreg = gen_reg_rtx (DFmode); + rtx clobreg = gen_reg_rtx (DImode); emit_insn (gen_no_literal_pool_df_immediate (operands[0], operands[1], clobreg)); DONE; diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md index 60e7ba35d8b..03514acc94f 100644 --- a/gcc/config/arm/vfp.md +++ b/gcc/config/arm/vfp.md @@ -2138,7 +2138,7 @@ (define_insn "get_fpscr" (define_insn_and_split "no_literal_pool_df_immediate" [(set (match_operand:DF 0 "s_register_operand" "=w") (match_operand:DF 1 "const_double_operand" "F")) - (clobber (match_operand:DF 2 "s_register_operand" "=r"))] + (clobber (match_operand:DI 2 "s_register_operand" "=r"))] "arm_disable_literal_pool && TARGET_VFP_BASE && !arm_const_double_rtx (operands[1]) @@ -2153,8 +2153,9 @@ (define_insn_and_split "no_literal_pool_df_immediate" unsigned HOST_WIDE_INT ival = zext_hwi (buf[order], 32); ival |= (zext_hwi (buf[1 - order], 32) << 32); rtx cst = gen_int_mode (ival, DImode); - emit_move_insn (simplify_gen_subreg (DImode, operands[2], DFmode, 0), cst); - emit_move_insn (operands[0], operands[2]); + emit_move_insn (operands[2], cst); + emit_move_insn (operands[0], + simplify_gen_subreg (DFmode, operands[2], DImode, 0)); DONE; } ) diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c new file mode 100644 index 00000000000..d797b790232 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c @@ -0,0 +1,4 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_hard_ok } */ +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mbig-endian -mpure-code" } */ +double f() { return 5.0; }
> -----Original Message----- > From: Alex Coplan <Alex.Coplan@arm.com> > Sent: Thursday, May 25, 2023 11:26 AM > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > Cc: gcc-patches@gcc.gnu.org; nickc@redhat.com; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan > <ramana.gcc@gmail.com> > Subject: Re: [PATCH] arm: Fix ICE due to infinite splitting [PR109800] > > Hi Kyrill, > > On 23/05/2023 11:14, Kyrylo Tkachov wrote: > > Hi Alex, > > diff --git a/gcc/testsuite/gcc.target/arm/pr109800.c > b/gcc/testsuite/gcc.target/arm/pr109800.c > > new file mode 100644 > > index 00000000000..71d1ede13dd > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/arm/pr109800.c > > @@ -0,0 +1,3 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp- > d16 -mbig-endian -mpure-code" } */ > > +double f() { return 5.0; } > > > > ... The arm testsuite options are kinda hard to get right with all the effective > targets and multilibs and such hardcoded abi and march options tend to > break in some target. > > I suggest you put this testcase in gcc.target/arm/pure-code and add a dg- > skip-if to skip the test if the multilib options specify a different float-abi. > > How about this instead: > > diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > new file mode 100644 > index 00000000000..d797b790232 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > @@ -0,0 +1,4 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target arm_hard_ok } */ > +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 - > mbig-endian -mpure-code" } */ > +double f() { return 5.0; } > > Full v2 patch attached. Thanks, looks better but I think you'll still want to have a dg-skip-if to avoid explicit -mfloat-abi=soft and -mfloat-abi=softfp in the multilib options. You can grep in that test directory for examples Kyrill > > Thanks, > Alex
> -----Original Message----- > From: Gcc-patches <gcc-patches- > bounces+kyrylo.tkachov=arm.com@gcc.gnu.org> On Behalf Of Kyrylo > Tkachov via Gcc-patches > Sent: Thursday, May 25, 2023 11:48 AM > To: Alex Coplan <Alex.Coplan@arm.com> > Cc: gcc-patches@gcc.gnu.org; nickc@redhat.com; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan > <ramana.gcc@gmail.com> > Subject: RE: [PATCH] arm: Fix ICE due to infinite splitting [PR109800] > > > > > -----Original Message----- > > From: Alex Coplan <Alex.Coplan@arm.com> > > Sent: Thursday, May 25, 2023 11:26 AM > > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > Cc: gcc-patches@gcc.gnu.org; nickc@redhat.com; Richard Earnshaw > > <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan > > <ramana.gcc@gmail.com> > > Subject: Re: [PATCH] arm: Fix ICE due to infinite splitting [PR109800] > > > > Hi Kyrill, > > > > On 23/05/2023 11:14, Kyrylo Tkachov wrote: > > > Hi Alex, > > > diff --git a/gcc/testsuite/gcc.target/arm/pr109800.c > > b/gcc/testsuite/gcc.target/arm/pr109800.c > > > new file mode 100644 > > > index 00000000000..71d1ede13dd > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.target/arm/pr109800.c > > > @@ -0,0 +1,3 @@ > > > +/* { dg-do compile } */ > > > +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp- > > d16 -mbig-endian -mpure-code" } */ > > > +double f() { return 5.0; } > > > > > > ... The arm testsuite options are kinda hard to get right with all the > effective > > targets and multilibs and such hardcoded abi and march options tend to > > break in some target. > > > I suggest you put this testcase in gcc.target/arm/pure-code and add a dg- > > skip-if to skip the test if the multilib options specify a different float-abi. > > > > How about this instead: > > > > diff --git a/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > > b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > > new file mode 100644 > > index 00000000000..d797b790232 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/arm/pure-code/pr109800.c > > @@ -0,0 +1,4 @@ > > +/* { dg-do compile } */ > > +/* { dg-require-effective-target arm_hard_ok } */ > > +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp- > d16 - > > mbig-endian -mpure-code" } */ > > +double f() { return 5.0; } > > > > Full v2 patch attached. > > Thanks, looks better but I think you'll still want to have a dg-skip-if to avoid > explicit -mfloat-abi=soft and -mfloat-abi=softfp in the multilib options. You > can grep in that test directory for examples Actually, as discussed offline this patch is okay as it has the arm_hard_ok check. Thanks, Kyrill > Kyrill > > > > > Thanks, > > Alex
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index cbfc4543531..40c4d848238 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -7555,7 +7555,7 @@ (define_expand "movdf" && !arm_const_double_rtx (operands[1]) && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))) { - rtx clobreg = gen_reg_rtx (DFmode); + rtx clobreg = gen_reg_rtx (DImode); emit_insn (gen_no_literal_pool_df_immediate (operands[0], operands[1], clobreg)); DONE; diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md index 60e7ba35d8b..03514acc94f 100644 --- a/gcc/config/arm/vfp.md +++ b/gcc/config/arm/vfp.md @@ -2138,7 +2138,7 @@ (define_insn "get_fpscr" (define_insn_and_split "no_literal_pool_df_immediate" [(set (match_operand:DF 0 "s_register_operand" "=w") (match_operand:DF 1 "const_double_operand" "F")) - (clobber (match_operand:DF 2 "s_register_operand" "=r"))] + (clobber (match_operand:DI 2 "s_register_operand" "=r"))] "arm_disable_literal_pool && TARGET_VFP_BASE && !arm_const_double_rtx (operands[1]) @@ -2153,8 +2153,9 @@ (define_insn_and_split "no_literal_pool_df_immediate" unsigned HOST_WIDE_INT ival = zext_hwi (buf[order], 32); ival |= (zext_hwi (buf[1 - order], 32) << 32); rtx cst = gen_int_mode (ival, DImode); - emit_move_insn (simplify_gen_subreg (DImode, operands[2], DFmode, 0), cst); - emit_move_insn (operands[0], operands[2]); + emit_move_insn (operands[2], cst); + emit_move_insn (operands[0], + simplify_gen_subreg (DFmode, operands[2], DImode, 0)); DONE; } ) diff --git a/gcc/testsuite/gcc.target/arm/pr109800.c b/gcc/testsuite/gcc.target/arm/pr109800.c new file mode 100644 index 00000000000..71d1ede13dd --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr109800.c @@ -0,0 +1,3 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -march=armv7-m -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mbig-endian -mpure-code" } */ +double f() { return 5.0; }