| Message ID | 6D39441BF12EF246A7ABCE6654B0235380B5CE7E@hhmail02.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On 02/07/2017 09:08 AM, Matthew Fortune wrote: > Hi, > > This change deals with reloading a subreg(reg) using the inner mode > to prevent partial spilling of data like in the case described here: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 > > No test case for now but I am investigating a targeted test using > the RTL frontend for later submission. > The patch is OK for the trunk. Thanks. > gcc/ > PR target/78660 > * lra-constraints.c (curr_insn_transform): Handle > WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. > --- > gcc/lra-constraints.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > index 22323b2..f29308f 100644 > --- a/gcc/lra-constraints.c > +++ b/gcc/lra-constraints.c > @@ -4130,7 +4130,14 @@ curr_insn_transform (bool check_only_p) > && (goal_alt[i] == NO_REGS > || (simplify_subreg_regno > (ira_class_hard_regs[goal_alt[i]][0], > - GET_MODE (reg), byte, mode) >= 0))))) > + GET_MODE (reg), byte, mode) >= 0))) > + /* WORD_REGISTER_OPERATIONS targets require the register > + to be reloaded when the outer mode is strictly > + narrower than the inner mode. Note: It may be > + necessary to always reload the inner mode here but it > + requires further investigation. */ > + || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg)) > + && WORD_REGISTER_OPERATIONS))) > { > if (type == OP_OUT) > type = OP_INOUT;
On 02/07/2017 09:08 AM, Matthew Fortune wrote: > Hi, > > This change deals with reloading a subreg(reg) using the inner mode > to prevent partial spilling of data like in the case described here: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 > > No test case for now but I am investigating a targeted test using > the RTL frontend for later submission. > > > gcc/ > PR target/78660 > * lra-constraints.c (curr_insn_transform): Handle > WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. > The patch is OK. So all 5 patches can be committed to the trunk. I hope Eric's test of the patches on SPARC will be successful. Thank you again for your efforts.
> PR target/78660 > * lra-constraints.c (curr_insn_transform): Handle > WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. > --- > gcc/lra-constraints.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > index 22323b2..f29308f 100644 > --- a/gcc/lra-constraints.c > +++ b/gcc/lra-constraints.c > @@ -4130,7 +4130,14 @@ curr_insn_transform (bool check_only_p) > && (goal_alt[i] == NO_REGS > > || (simplify_subreg_regno > > (ira_class_hard_regs[goal_alt[i]][0], > - GET_MODE (reg), byte, mode) >= 0))))) > + GET_MODE (reg), byte, mode) >= 0))) > + /* WORD_REGISTER_OPERATIONS targets require the register > + to be reloaded when the outer mode is strictly > + narrower than the inner mode. Note: It may be > + necessary to always reload the inner mode here but it > + requires further investigation. */ > + || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg)) > + && WORD_REGISTER_OPERATIONS))) > { > if (type == OP_OUT) > type = OP_INOUT; You want GET_MODE_PRECISION instead of GET_MODE_SIZE here.
Vladimir Makarov <vmakarov@redhat.com> writes: > On 02/07/2017 09:08 AM, Matthew Fortune wrote: > > Hi, > > > > This change deals with reloading a subreg(reg) using the inner mode to > > prevent partial spilling of data like in the case described here: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 > > > > No test case for now but I am investigating a targeted test using the > > RTL frontend for later submission. > > > > > > gcc/ > > PR target/78660 > > * lra-constraints.c (curr_insn_transform): Handle > > WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. > > > The patch is OK. So all 5 patches can be committed to the trunk. I > hope Eric's test of the patches on SPARC will be successful. Thank you > again for your efforts. Committed as r245598. Thanks, Matthew
Hi, On 20 February 2017 at 13:08, Matthew Fortune <Matthew.Fortune@imgtec.com> wrote: > Vladimir Makarov <vmakarov@redhat.com> writes: >> On 02/07/2017 09:08 AM, Matthew Fortune wrote: >> > Hi, >> > >> > This change deals with reloading a subreg(reg) using the inner mode to >> > prevent partial spilling of data like in the case described here: >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 >> > >> > No test case for now but I am investigating a targeted test using the >> > RTL frontend for later submission. >> > >> > >> > gcc/ >> > PR target/78660 >> > * lra-constraints.c (curr_insn_transform): Handle >> > WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. >> > >> The patch is OK. So all 5 patches can be committed to the trunk. I >> hope Eric's test of the patches on SPARC will be successful. Thank you >> again for your efforts. > > Committed as r245598. > This patch is causing ICEs on arm-none-linux-gnueabi FAIL: gcc.c-torture/execute/simd-2.c -O1 (internal compiler error) /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c: In function 'main': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: error: unable to find a register to spill /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: error: this is the insn: (insn 276 2123 2129 2 (set (subreg:SI (reg:TI 886 [362]) 0) (and:SI (subreg:SI (reg:TI 567 [orig:111 j.1_2 ] [111]) 0) (subreg:SI (reg:TI 568 [orig:110 i.0_1 ] [110]) 0))) "/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c":47 82 {*arm_andsi3_insn} (expr_list:REG_DEAD (reg:TI 568 [orig:110 i.0_1 ] [110]) (expr_list:REG_DEAD (reg:TI 567 [orig:111 j.1_2 ] [111]) (nil)))) /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: internal compiler error: in assign_by_spills, at lra-assigns.c:1457 0xacc9d3 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 0x9a6123 assign_by_spills /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 0x9a7506 lra_assign() /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 0x9a16d4 lra(_IO_FILE*) /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 0x957669 do_reload /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 0x957669 execute /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 I've also noticed that --target arm-none-eabi with default --with-mode and --with-cpu fails to build libgcc (it may be easier to reproduce): /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c: In function '__gnu_mulhelperudq': /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: error: unable to find a register to spill } ^ /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: error: this is the insn: (insn 286 296 287 2 (set (reg:SI 232) (neg:SI (ltu:SI (subreg:SI (reg:DI 238 [orig:119 _10 ] [119]) 4) (subreg:SI (reg/v:DI 149 [ temp1 ]) 4)))) "/tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c":314 849 {cstor esi_nltu_thumb1} (expr_list:REG_DEAD (reg:DI 238 [orig:119 _10 ] [119]) (nil))) /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: internal compiler error: in assign_by_spills, at lra-assigns.c:1457 0xacc9c3 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 0x9a6113 assign_by_spills /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 0x9a74f6 lra_assign() /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 0x9a16c4 lra(_IO_FILE*) /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 0x957659 do_reload /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 0x957659 execute /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 make[4]: *** [_mulhelperUDQ.o] Error 1 Thanks, Christophe > Thanks, > Matthew
On 21/02/17 10:54, Christophe Lyon wrote: > Hi, > > On 20 February 2017 at 13:08, Matthew Fortune > <Matthew.Fortune@imgtec.com> wrote: >> Vladimir Makarov <vmakarov@redhat.com> writes: >>> On 02/07/2017 09:08 AM, Matthew Fortune wrote: >>>> Hi, >>>> >>>> This change deals with reloading a subreg(reg) using the inner mode to >>>> prevent partial spilling of data like in the case described here: >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 >>>> >>>> No test case for now but I am investigating a targeted test using the >>>> RTL frontend for later submission. >>>> >>>> >>>> gcc/ >>>> PR target/78660 >>>> * lra-constraints.c (curr_insn_transform): Handle >>>> WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. >>>> >>> The patch is OK. So all 5 patches can be committed to the trunk. I >>> hope Eric's test of the patches on SPARC will be successful. Thank you >>> again for your efforts. >> Committed as r245598. >> > This patch is causing ICEs on arm-none-linux-gnueabi > FAIL: gcc.c-torture/execute/simd-2.c -O1 (internal compiler error) > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c: > In function 'main': > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: > error: unable to find a register to spill > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: > error: this is the insn: > (insn 276 2123 2129 2 (set (subreg:SI (reg:TI 886 [362]) 0) > (and:SI (subreg:SI (reg:TI 567 [orig:111 j.1_2 ] [111]) 0) > (subreg:SI (reg:TI 568 [orig:110 i.0_1 ] [110]) 0))) > "/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c":47 > 82 {*arm_andsi3_insn} > (expr_list:REG_DEAD (reg:TI 568 [orig:110 i.0_1 ] [110]) > (expr_list:REG_DEAD (reg:TI 567 [orig:111 j.1_2 ] [111]) > (nil)))) > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: > internal compiler error: in assign_by_spills, at lra-assigns.c:1457 > 0xacc9d3 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 > 0x9a6123 assign_by_spills > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 > 0x9a7506 lra_assign() > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 > 0x9a16d4 lra(_IO_FILE*) > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 > 0x957669 do_reload > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 > 0x957669 execute > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 > > > > I've also noticed that --target arm-none-eabi with default --with-mode > and --with-cpu > fails to build libgcc (it may be easier to reproduce): > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c: > In function '__gnu_mulhelperudq': > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: > error: unable to find a register to spill > } > ^ > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: > error: this is the insn: > (insn 286 296 287 2 (set (reg:SI 232) > (neg:SI (ltu:SI (subreg:SI (reg:DI 238 [orig:119 _10 ] [119]) 4) > (subreg:SI (reg/v:DI 149 [ temp1 ]) 4)))) > "/tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c":314 > 849 {cstor > esi_nltu_thumb1} > (expr_list:REG_DEAD (reg:DI 238 [orig:119 _10 ] [119]) > (nil))) This looks like PR 79660 that Richard just filed. Kyrill > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: > internal compiler error: in assign_by_spills, at lra-assigns.c:1457 > 0xacc9c3 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 > 0x9a6113 assign_by_spills > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 > 0x9a74f6 lra_assign() > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 > 0x9a16c4 lra(_IO_FILE*) > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 > 0x957659 do_reload > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 > 0x957659 execute > /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 > make[4]: *** [_mulhelperUDQ.o] Error 1 > > Thanks, > > Christophe > > >> Thanks, >> Matthew
On 21 February 2017 at 11:59, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > > On 21/02/17 10:54, Christophe Lyon wrote: >> >> Hi, >> >> On 20 February 2017 at 13:08, Matthew Fortune >> <Matthew.Fortune@imgtec.com> wrote: >>> >>> Vladimir Makarov <vmakarov@redhat.com> writes: >>>> >>>> On 02/07/2017 09:08 AM, Matthew Fortune wrote: >>>>> >>>>> Hi, >>>>> >>>>> This change deals with reloading a subreg(reg) using the inner mode to >>>>> prevent partial spilling of data like in the case described here: >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 >>>>> >>>>> No test case for now but I am investigating a targeted test using the >>>>> RTL frontend for later submission. >>>>> >>>>> >>>>> gcc/ >>>>> PR target/78660 >>>>> * lra-constraints.c (curr_insn_transform): Handle >>>>> WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. >>>>> >>>> The patch is OK. So all 5 patches can be committed to the trunk. I >>>> hope Eric's test of the patches on SPARC will be successful. Thank you >>>> again for your efforts. >>> >>> Committed as r245598. >>> >> This patch is causing ICEs on arm-none-linux-gnueabi >> FAIL: gcc.c-torture/execute/simd-2.c -O1 (internal compiler error) >> >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c: >> In function 'main': >> >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: >> error: unable to find a register to spill >> >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: >> error: this is the insn: >> (insn 276 2123 2129 2 (set (subreg:SI (reg:TI 886 [362]) 0) >> (and:SI (subreg:SI (reg:TI 567 [orig:111 j.1_2 ] [111]) 0) >> (subreg:SI (reg:TI 568 [orig:110 i.0_1 ] [110]) 0))) >> >> "/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c":47 >> 82 {*arm_andsi3_insn} >> (expr_list:REG_DEAD (reg:TI 568 [orig:110 i.0_1 ] [110]) >> (expr_list:REG_DEAD (reg:TI 567 [orig:111 j.1_2 ] [111]) >> (nil)))) >> >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/execute/simd-2.c:72:1: >> internal compiler error: in assign_by_spills, at lra-assigns.c:1457 >> 0xacc9d3 _fatal_insn(char const*, rtx_def const*, char const*, int, char >> const*) >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 >> 0x9a6123 assign_by_spills >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 >> 0x9a7506 lra_assign() >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 >> 0x9a16d4 lra(_IO_FILE*) >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 >> 0x957669 do_reload >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 >> 0x957669 execute >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 >> >> >> >> I've also noticed that --target arm-none-eabi with default --with-mode >> and --with-cpu >> fails to build libgcc (it may be easier to reproduce): >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c: >> In function '__gnu_mulhelperudq': >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: >> error: unable to find a register to spill >> } >> ^ >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: >> error: this is the insn: >> (insn 286 296 287 2 (set (reg:SI 232) >> (neg:SI (ltu:SI (subreg:SI (reg:DI 238 [orig:119 _10 ] [119]) 4) >> (subreg:SI (reg/v:DI 149 [ temp1 ]) 4)))) >> >> "/tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c":314 >> 849 {cstor >> esi_nltu_thumb1} >> (expr_list:REG_DEAD (reg:DI 238 [orig:119 _10 ] [119]) >> (nil))) > > > This looks like PR 79660 that Richard just filed. Indeed, thanks. > Kyrill > > >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixed-bit.c:371:1: >> internal compiler error: in assign_by_spills, at lra-assigns.c:1457 >> 0xacc9c3 _fatal_insn(char const*, rtx_def const*, char const*, int, char >> const*) >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 >> 0x9a6113 assign_by_spills >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 >> 0x9a74f6 lra_assign() >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 >> 0x9a16c4 lra(_IO_FILE*) >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 >> 0x957659 do_reload >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 >> 0x957659 execute >> >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 >> make[4]: *** [_mulhelperUDQ.o] Error 1 >> >> Thanks, >> >> Christophe >> >> >>> Thanks, >>> Matthew > >
Christophe Lyon <christophe.lyon@linaro.org> writes: > On 21 February 2017 at 11:59, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: > > > > On 21/02/17 10:54, Christophe Lyon wrote: > >> > >> Hi, > >> > >> On 20 February 2017 at 13:08, Matthew Fortune > >> <Matthew.Fortune@imgtec.com> wrote: > >>> > >>> Vladimir Makarov <vmakarov@redhat.com> writes: > >>>> > >>>> On 02/07/2017 09:08 AM, Matthew Fortune wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> This change deals with reloading a subreg(reg) using the inner > >>>>> mode to prevent partial spilling of data like in the case > described here: > >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78660#c8 > >>>>> > >>>>> No test case for now but I am investigating a targeted test using > >>>>> the RTL frontend for later submission. > >>>>> > >>>>> > >>>>> gcc/ > >>>>> PR target/78660 > >>>>> * lra-constraints.c (curr_insn_transform): Handle > >>>>> WORD_REGISTER_OPERATIONS requirements when reloading SUBREGs. > >>>>> > >>>> The patch is OK. So all 5 patches can be committed to the trunk. > >>>> I hope Eric's test of the patches on SPARC will be successful. > >>>> Thank you again for your efforts. > >>> > >>> Committed as r245598. > >>> > >> This patch is causing ICEs on arm-none-linux-gnueabi > >> FAIL: gcc.c-torture/execute/simd-2.c -O1 (internal compiler error) > >> > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c- > torture/execute/simd-2.c: > >> In function 'main': > >> > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c- > torture/execute/simd-2.c:72:1: > >> error: unable to find a register to spill > >> > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c- > torture/execute/simd-2.c:72:1: > >> error: this is the insn: > >> (insn 276 2123 2129 2 (set (subreg:SI (reg:TI 886 [362]) 0) > >> (and:SI (subreg:SI (reg:TI 567 [orig:111 j.1_2 ] [111]) 0) > >> (subreg:SI (reg:TI 568 [orig:110 i.0_1 ] [110]) 0))) > >> > >> "/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c-torture/exec > >> ute/simd-2.c":47 > >> 82 {*arm_andsi3_insn} > >> (expr_list:REG_DEAD (reg:TI 568 [orig:110 i.0_1 ] [110]) > >> (expr_list:REG_DEAD (reg:TI 567 [orig:111 j.1_2 ] [111]) > >> (nil)))) > >> > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/gcc.c- > torture/execute/simd-2.c:72:1: > >> internal compiler error: in assign_by_spills, at lra-assigns.c:1457 > >> 0xacc9d3 _fatal_insn(char const*, rtx_def const*, char const*, int, > >> char > >> const*) > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-error.c:108 > >> 0x9a6123 assign_by_spills > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1457 > >> 0x9a7506 lra_assign() > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assigns.c:1643 > >> 0x9a16d4 lra(_IO_FILE*) > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:2451 > >> 0x957669 do_reload > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5452 > >> 0x957669 execute > >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:5636 > >> > >> > >> > >> I've also noticed that --target arm-none-eabi with default > >> --with-mode and --with-cpu fails to build libgcc (it may be easier to > >> reproduce): > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc- > fsf/gccsrc/libgcc/fixed-bit.c: > >> In function '__gnu_mulhelperudq': > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc- > fsf/gccsrc/libgcc/fixed-bit.c:371:1: > >> error: unable to find a register to spill > >> } > >> ^ > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc- > fsf/gccsrc/libgcc/fixed-bit.c:371:1: > >> error: this is the insn: > >> (insn 286 296 287 2 (set (reg:SI 232) > >> (neg:SI (ltu:SI (subreg:SI (reg:DI 238 [orig:119 _10 ] > [119]) 4) > >> (subreg:SI (reg/v:DI 149 [ temp1 ]) 4)))) > >> > >> "/tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/fixe > >> d-bit.c":314 > >> 849 {cstor > >> esi_nltu_thumb1} > >> (expr_list:REG_DEAD (reg:DI 238 [orig:119 _10 ] [119]) > >> (nil))) > > > > > > This looks like PR 79660 that Richard just filed. > > Indeed, thanks. Thanks for reporting. I'll take a brief look first but revert if the issue isn't something that vaguely makes sense to me. Sorry for the hassle. Matthew > > > Kyrill > > > > > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc- > fsf/gccsrc/libgcc/fixed-bit.c:371:1: > >> internal compiler error: in assign_by_spills, at lra-assigns.c:1457 > >> 0xacc9c3 _fatal_insn(char const*, rtx_def const*, char const*, int, > >> char > >> const*) > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/rtl-erro > >> r.c:108 > >> 0x9a6113 assign_by_spills > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assi > >> gns.c:1457 > >> 0x9a74f6 lra_assign() > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra-assi > >> gns.c:1643 > >> 0x9a16c4 lra(_IO_FILE*) > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/lra.c:24 > >> 51 > >> 0x957659 do_reload > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:54 > >> 52 > >> 0x957659 execute > >> > >> /tmp/870921_29.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/ira.c:56 > >> 36 > >> make[4]: *** [_mulhelperUDQ.o] Error 1 > >> > >> Thanks, > >> > >> Christophe > >> > >> > >>> Thanks, > >>> Matthew > > > >
> Thanks for reporting. I'll take a brief look first but revert if the > issue isn't something that vaguely makes sense to me. You need to restrict any WORD_REGISTER_OPERATIONS test to subword registers.
Eric Botcazou <ebotcazou@adacore.com> writes: > > Thanks for reporting. I'll take a brief look first but revert if the > > issue isn't something that vaguely makes sense to me. > > You need to restrict any WORD_REGISTER_OPERATIONS test to subword > registers. I've reverted this. I haven't been able to quickly find a change that I both feel is right and works. I am wondering if I actually don't need this change and that 'patch 3' (the change to simplify_operand_subreg) is the actual fix for both issues I have seen. I'll test my other change against an ARM build and wait a day to see that ARM is at least working on trunk before committing the other fix to this area of code. Thanks, Matthew
On 21/02/17 16:58, Matthew Fortune wrote: > Eric Botcazou <ebotcazou@adacore.com> writes: >>> Thanks for reporting. I'll take a brief look first but revert if the >>> issue isn't something that vaguely makes sense to me. >> You need to restrict any WORD_REGISTER_OPERATIONS test to subword >> registers. > I've reverted this. I haven't been able to quickly find a change that > I both feel is right and works. I am wondering if I actually don't > need this change and that 'patch 3' (the change to > simplify_operand_subreg) is the actual fix for both issues I have seen. > > I'll test my other change against an ARM build and wait a day to see > that ARM is at least working on trunk before committing the other > fix to this area of code. Thanks Matthew. Kyrill > Thanks, > Matthew
Matthew Fortune <Matthew.Fortune@imgtec.com> writes: > Eric Botcazou <ebotcazou@adacore.com> writes: >> > Thanks for reporting. I'll take a brief look first but revert if the >> > issue isn't something that vaguely makes sense to me. >> >> You need to restrict any WORD_REGISTER_OPERATIONS test to subword >> registers. > > I've reverted this. I haven't been able to quickly find a change that > I both feel is right and works. I am wondering if I actually don't > need this change and that 'patch 3' (the change to > simplify_operand_subreg) is the actual fix for both issues I have seen. I think that patch might have a similar problem though, in that it applies WORD_REGISTER_OPERATIONS semantics to things that might be vectors. Thanks, Richard
Richard Sandiford <richard.sandiford@arm.com> writes: > Matthew Fortune <Matthew.Fortune@imgtec.com> writes: > > Eric Botcazou <ebotcazou@adacore.com> writes: > >> > Thanks for reporting. I'll take a brief look first but revert if > >> > the issue isn't something that vaguely makes sense to me. > >> > >> You need to restrict any WORD_REGISTER_OPERATIONS test to subword > >> registers. > > > > I've reverted this. I haven't been able to quickly find a change that > > I both feel is right and works. I am wondering if I actually don't > > need this change and that 'patch 3' (the change to > > simplify_operand_subreg) is the actual fix for both issues I have > seen. > > I think that patch might have a similar problem though, in that it > applies WORD_REGISTER_OPERATIONS semantics to things that might be > vectors. There is an amendment done as part of SPARC testing that limits the change to modes that are no wider than a word. But, given that assumptions coming from WORD_REGISTER_OPERATIONS can only be applied to integer modes then it should also check both modes are MODE_INT as well. All that said... I don't entirely follow why any target should be reliant on subreg(mem) being simplified to use the outer mode. It is an optimization certainly for some cases but I don't understand what rule or guarantee we have that means reloading the inner MEM is illegal. The latter point is not appropriate to debate for GCC 7 though. Matthew
Matthew Fortune <Matthew.Fortune@imgtec.com> writes: > Richard Sandiford <richard.sandiford@arm.com> writes: >> Matthew Fortune <Matthew.Fortune@imgtec.com> writes: >> > Eric Botcazou <ebotcazou@adacore.com> writes: >> >> > Thanks for reporting. I'll take a brief look first but revert if >> >> > the issue isn't something that vaguely makes sense to me. >> >> >> >> You need to restrict any WORD_REGISTER_OPERATIONS test to subword >> >> registers. >> > >> > I've reverted this. I haven't been able to quickly find a change that >> > I both feel is right and works. I am wondering if I actually don't >> > need this change and that 'patch 3' (the change to >> > simplify_operand_subreg) is the actual fix for both issues I have >> seen. >> >> I think that patch might have a similar problem though, in that it >> applies WORD_REGISTER_OPERATIONS semantics to things that might be >> vectors. > > There is an amendment done as part of SPARC testing that limits the > change to modes that are no wider than a word. But, given that assumptions > coming from WORD_REGISTER_OPERATIONS can only be applied to integer > modes then it should also check both modes are MODE_INT as well. Oops, sorry, I should have checked. > All that said... I don't entirely follow why any target should be > reliant on subreg(mem) being simplified to use the outer mode. It is an > optimization certainly for some cases but I don't understand what rule > or guarantee we have that means reloading the inner MEM is illegal. Agreed. I don't think things like WORD_MODE_OPERATIONS should change rtl semantics, just optimisation decisions. And using the smallest possible spill size is often good even for RISCy targets. > The latter point is not appropriate to debate for GCC 7 though. Yeah. Thanks, Richard
> Agreed. I don't think things like WORD_MODE_OPERATIONS should change > rtl semantics, just optimisation decisions. And using the smallest > possible spill size is often good even for RISCy targets. I don't think that's correct, WORD_MODE_OPERATIONS does change RTL semantics for SUBREGs smaller than a word since it can make all bits defined, even if only the lower part is assigned. For example (SUBREG:SI (MEM:QI)) has the higher bits defined according to LOAD_EXTEND_OP if WORD_MODE_OPERATIONS. LRA simply needs to preserve the semantics, just as reload does.
Eric Botcazou <ebotcazou@adacore.com> writes: > > Agreed. I don't think things like WORD_MODE_OPERATIONS should change > > rtl semantics, just optimisation decisions. Sorry, I did cover two topics in one email. My point was about whether simplifying: (subreg:OUTER (mem:INNER ...)) To: (mem:OUTER ...) Should ever be a requirement for successful reloading rather than just an optimisation that 'could' be applied. There are a few cases it seems that require this simplification to happen which I find odd. > > And using the smallest > > possible spill size is often good even for RISCy targets. Yes, if (subreg(mem)) simplification only ever happened when it was reducing the size of the load/store I would understand the code more too but actually it will currently happily simplify to a wider mode. Using a wider mode may well be beneficial in other situations where a further reload would be needed due to insn constraints I guess. It all feels a bit like magic still. > I don't think that's correct, WORD_MODE_OPERATIONS does change RTL semantics > for SUBREGs smaller than a word since it can make all bits defined, even if > only the lower part is assigned. For example (SUBREG:SI (MEM:QI)) has the > higher bits defined according to LOAD_EXTEND_OP if WORD_MODE_OPERATIONS. It would almost be simpler if we had another variant of subreg (like we have strict_low_part) to explicitly represent the WORD_REGISTER_OPERATIONS behaviour with the mode change. I'll stop myself and hold this for later though! > LRA simply needs to preserve the semantics, just as reload does. I will keep working on this code post GCC 7 as the topic is bugging me now :-) Thanks, Matthew
On 21/02/17 19:53, Matthew Fortune wrote: > Eric Botcazou <ebotcazou@adacore.com> writes: >>> Agreed. I don't think things like WORD_MODE_OPERATIONS should change >>> rtl semantics, just optimisation decisions. > > Sorry, I did cover two topics in one email. My point was about whether > simplifying: > > (subreg:OUTER (mem:INNER ...)) > To: > (mem:OUTER ...) > > Should ever be a requirement for successful reloading rather than just an > optimisation that 'could' be applied. There are a few cases it seems that > require this simplification to happen which I find odd. > If mem:INNER has side effects (volatile, pre/post addressing etc), then the mem must never be accessed in any mode other than INNER. So I agree that if there are places that assume otherwise that is odd; but they may have already dealt with the side effects problem earlier. R. >>> And using the smallest >>> possible spill size is often good even for RISCy targets. > > Yes, if (subreg(mem)) simplification only ever happened when it was reducing > the size of the load/store I would understand the code more too but actually > it will currently happily simplify to a wider mode. Using a wider mode may > well be beneficial in other situations where a further reload would be needed > due to insn constraints I guess. It all feels a bit like magic still. > >> I don't think that's correct, WORD_MODE_OPERATIONS does change RTL semantics >> for SUBREGs smaller than a word since it can make all bits defined, even if >> only the lower part is assigned. For example (SUBREG:SI (MEM:QI)) has the >> higher bits defined according to LOAD_EXTEND_OP if WORD_MODE_OPERATIONS. > > It would almost be simpler if we had another variant of subreg (like we have > strict_low_part) to explicitly represent the WORD_REGISTER_OPERATIONS behaviour > with the mode change. I'll stop myself and hold this for later though! > >> LRA simply needs to preserve the semantics, just as reload does. > > I will keep working on this code post GCC 7 as the topic is bugging me now :-) > > Thanks, > Matthew >
> I will keep working on this code post GCC 7 as the topic is bugging me now > :-) I'm OK to lend a hand, but what's left for GCC 7 to make MIPS happy?
> > I will keep working on this code post GCC 7 as the topic is bugging me > now > > :-) > > I'm OK to lend a hand, but what's left for GCC 7 to make MIPS happy? I have just successfully bootstrapped MIPS with just the pending (amended) patch (3). i.e. with this patch (1) reverted so I did not need this one after all. I should have gone back and checked that originally. I'll commit the updated patch (3) later today. After checking an ARM build. Thanks for everyone's help and patience, Matthew
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index 22323b2..f29308f 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -4130,7 +4130,14 @@ curr_insn_transform (bool check_only_p) && (goal_alt[i] == NO_REGS || (simplify_subreg_regno (ira_class_hard_regs[goal_alt[i]][0], - GET_MODE (reg), byte, mode) >= 0))))) + GET_MODE (reg), byte, mode) >= 0))) + /* WORD_REGISTER_OPERATIONS targets require the register + to be reloaded when the outer mode is strictly + narrower than the inner mode. Note: It may be + necessary to always reload the inner mode here but it + requires further investigation. */ + || (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (reg)) + && WORD_REGISTER_OPERATIONS))) { if (type == OP_OUT) type = OP_INOUT;