[PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Vladimir Makarov]

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;

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Vladimir Makarov]

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.

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Eric Botcazou]

> 	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

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Christophe Lyon]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Kyrill Tkachov]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Christophe Lyon]

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
>
>

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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
> >
> >

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Eric Botcazou]

> 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

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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 

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Kyrill Tkachov]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Richard Sandiford]

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

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Richard Sandiford]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Eric Botcazou]

> 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

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

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

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Richard Earnshaw (lists)]

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
> 

Re: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Eric Botcazou]

> 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

RE: [PATCH 1/5] Handle WORD_REGISTER_OPERATIONS when reloading (subreg(reg))

[Matthew Fortune]

> > 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