* [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
@ 2017-02-07 14:08 Matthew Fortune
2017-02-07 22:10 ` Vladimir Makarov
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Matthew Fortune @ 2017-02-07 14:08 UTC (permalink / raw)
To: 'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)
Cc: Vladimir Makarov, Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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.
Thanks,
Matthew
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;
--
2.2.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-07 14:08 [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg)) Matthew Fortune
@ 2017-02-07 22:10 ` Vladimir Makarov
2017-02-07 22:18 ` Vladimir Makarov
2017-02-08 15:03 ` Eric Botcazou
2 siblings, 0 replies; 20+ messages in thread
From: Vladimir Makarov @ 2017-02-07 22:10 UTC (permalink / raw)
To: Matthew Fortune,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)
Cc: Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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;
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-07 14:08 [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg)) Matthew Fortune
2017-02-07 22:10 ` Vladimir Makarov
@ 2017-02-07 22:18 ` Vladimir Makarov
2017-02-20 12:09 ` Matthew Fortune
2017-02-08 15:03 ` Eric Botcazou
2 siblings, 1 reply; 20+ messages in thread
From: Vladimir Makarov @ 2017-02-07 22:18 UTC (permalink / raw)
To: Matthew Fortune,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)
Cc: Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-07 14:08 [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg)) Matthew Fortune
2017-02-07 22:10 ` Vladimir Makarov
2017-02-07 22:18 ` Vladimir Makarov
@ 2017-02-08 15:03 ` Eric Botcazou
2 siblings, 0 replies; 20+ messages in thread
From: Eric Botcazou @ 2017-02-08 15:03 UTC (permalink / raw)
To: Matthew Fortune
Cc: gcc-patches, Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
> 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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-07 22:18 ` Vladimir Makarov
@ 2017-02-20 12:09 ` Matthew Fortune
2017-02-21 10:59 ` Christophe Lyon
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Fortune @ 2017-02-20 12:09 UTC (permalink / raw)
To: Vladimir Makarov,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)
Cc: Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-20 12:09 ` Matthew Fortune
@ 2017-02-21 10:59 ` Christophe Lyon
2017-02-21 11:03 ` Kyrill Tkachov
0 siblings, 1 reply; 20+ messages in thread
From: Christophe Lyon @ 2017-02-21 10:59 UTC (permalink / raw)
To: Matthew Fortune
Cc: Vladimir Makarov,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 10:59 ` Christophe Lyon
@ 2017-02-21 11:03 ` Kyrill Tkachov
2017-02-21 11:13 ` Christophe Lyon
0 siblings, 1 reply; 20+ messages in thread
From: Kyrill Tkachov @ 2017-02-21 11:03 UTC (permalink / raw)
To: Christophe Lyon, Matthew Fortune
Cc: Vladimir Makarov,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 11:03 ` Kyrill Tkachov
@ 2017-02-21 11:13 ` Christophe Lyon
2017-02-21 11:20 ` Matthew Fortune
0 siblings, 1 reply; 20+ messages in thread
From: Christophe Lyon @ 2017-02-21 11:13 UTC (permalink / raw)
To: Kyrill Tkachov
Cc: Matthew Fortune, Vladimir Makarov,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 11:13 ` Christophe Lyon
@ 2017-02-21 11:20 ` Matthew Fortune
2017-02-21 11:49 ` Eric Botcazou
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Fortune @ 2017-02-21 11:20 UTC (permalink / raw)
To: Christophe Lyon, Kyrill Tkachov
Cc: Vladimir Makarov,
'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org),
Eric Botcazou (ebotcazou@adacore.com),
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
> >
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 11:20 ` Matthew Fortune
@ 2017-02-21 11:49 ` Eric Botcazou
2017-02-21 17:04 ` Matthew Fortune
0 siblings, 1 reply; 20+ messages in thread
From: Eric Botcazou @ 2017-02-21 11:49 UTC (permalink / raw)
To: Matthew Fortune
Cc: gcc-patches, Christophe Lyon, Kyrill Tkachov, Vladimir Makarov,
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 11:49 ` Eric Botcazou
@ 2017-02-21 17:04 ` Matthew Fortune
2017-02-21 17:16 ` Kyrill Tkachov
2017-02-21 17:22 ` Richard Sandiford
0 siblings, 2 replies; 20+ messages in thread
From: Matthew Fortune @ 2017-02-21 17:04 UTC (permalink / raw)
To: Eric Botcazou
Cc: gcc-patches, Christophe Lyon, Kyrill Tkachov, Vladimir Makarov,
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 17:04 ` Matthew Fortune
@ 2017-02-21 17:16 ` Kyrill Tkachov
2017-02-21 17:22 ` Richard Sandiford
1 sibling, 0 replies; 20+ messages in thread
From: Kyrill Tkachov @ 2017-02-21 17:16 UTC (permalink / raw)
To: Matthew Fortune, Eric Botcazou
Cc: gcc-patches, Christophe Lyon, Kyrill Tkachov, Vladimir Makarov,
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 17:04 ` Matthew Fortune
2017-02-21 17:16 ` Kyrill Tkachov
@ 2017-02-21 17:22 ` Richard Sandiford
2017-02-21 17:41 ` Matthew Fortune
1 sibling, 1 reply; 20+ messages in thread
From: Richard Sandiford @ 2017-02-21 17:22 UTC (permalink / raw)
To: Matthew Fortune
Cc: Eric Botcazou, gcc-patches, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 17:22 ` Richard Sandiford
@ 2017-02-21 17:41 ` Matthew Fortune
2017-02-21 18:33 ` Richard Sandiford
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Fortune @ 2017-02-21 17:41 UTC (permalink / raw)
To: Richard Sandiford
Cc: Eric Botcazou, gcc-patches, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 17:41 ` Matthew Fortune
@ 2017-02-21 18:33 ` Richard Sandiford
2017-02-21 18:46 ` Eric Botcazou
0 siblings, 1 reply; 20+ messages in thread
From: Richard Sandiford @ 2017-02-21 18:33 UTC (permalink / raw)
To: Matthew Fortune
Cc: Eric Botcazou, gcc-patches, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 18:33 ` Richard Sandiford
@ 2017-02-21 18:46 ` Eric Botcazou
2017-02-21 20:08 ` Matthew Fortune
0 siblings, 1 reply; 20+ messages in thread
From: Eric Botcazou @ 2017-02-21 18:46 UTC (permalink / raw)
To: Richard Sandiford
Cc: gcc-patches, Matthew Fortune, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 18:46 ` Eric Botcazou
@ 2017-02-21 20:08 ` Matthew Fortune
2017-02-22 11:00 ` Richard Earnshaw (lists)
2017-02-22 11:15 ` Eric Botcazou
0 siblings, 2 replies; 20+ messages in thread
From: Matthew Fortune @ 2017-02-21 20:08 UTC (permalink / raw)
To: Eric Botcazou, Richard Sandiford
Cc: gcc-patches, Christophe Lyon, Kyrill Tkachov, Vladimir Makarov,
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 20:08 ` Matthew Fortune
@ 2017-02-22 11:00 ` Richard Earnshaw (lists)
2017-02-22 11:15 ` Eric Botcazou
1 sibling, 0 replies; 20+ messages in thread
From: Richard Earnshaw (lists) @ 2017-02-22 11:00 UTC (permalink / raw)
To: Matthew Fortune, Eric Botcazou, Richard Sandiford
Cc: gcc-patches, Christophe Lyon, Kyrill Tkachov, Vladimir Makarov,
Robert Suchanek, Moore, Catherine (Catherine_Moore@mentor.com)
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-21 20:08 ` Matthew Fortune
2017-02-22 11:00 ` Richard Earnshaw (lists)
@ 2017-02-22 11:15 ` Eric Botcazou
2017-02-22 11:32 ` Matthew Fortune
1 sibling, 1 reply; 20+ messages in thread
From: Eric Botcazou @ 2017-02-22 11:15 UTC (permalink / raw)
To: Matthew Fortune
Cc: gcc-patches, Richard Sandiford, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
> 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?
--
Eric Botcazou
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))
2017-02-22 11:15 ` Eric Botcazou
@ 2017-02-22 11:32 ` Matthew Fortune
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Fortune @ 2017-02-22 11:32 UTC (permalink / raw)
To: Eric Botcazou
Cc: gcc-patches, Richard Sandiford, Christophe Lyon, Kyrill Tkachov,
Vladimir Makarov, Robert Suchanek, Moore,
Catherine (Catherine_Moore@mentor.com)
> > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-02-22 11:30 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 14:08 [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg)) Matthew Fortune
2017-02-07 22:10 ` Vladimir Makarov
2017-02-07 22:18 ` Vladimir Makarov
2017-02-20 12:09 ` Matthew Fortune
2017-02-21 10:59 ` Christophe Lyon
2017-02-21 11:03 ` Kyrill Tkachov
2017-02-21 11:13 ` Christophe Lyon
2017-02-21 11:20 ` Matthew Fortune
2017-02-21 11:49 ` Eric Botcazou
2017-02-21 17:04 ` Matthew Fortune
2017-02-21 17:16 ` Kyrill Tkachov
2017-02-21 17:22 ` Richard Sandiford
2017-02-21 17:41 ` Matthew Fortune
2017-02-21 18:33 ` Richard Sandiford
2017-02-21 18:46 ` Eric Botcazou
2017-02-21 20:08 ` Matthew Fortune
2017-02-22 11:00 ` Richard Earnshaw (lists)
2017-02-22 11:15 ` Eric Botcazou
2017-02-22 11:32 ` Matthew Fortune
2017-02-08 15:03 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).