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

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

[Richard Earnshaw (lists)]

On 21/02/17 19:53, Matthew Fortune wrote:
> Eric Botcazou  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))

[Matthew Fortune]

Eric Botcazou  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))

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

[Richard Sandiford]

Matthew Fortune  writes:
> Richard Sandiford  writes:
>> Matthew Fortune  writes:
>> > Eric Botcazou  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))

[Matthew Fortune]

Richard Sandiford  writes:
> Matthew Fortune  writes:
> > Eric Botcazou  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  writes:
> Eric Botcazou  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))

[Kyrill Tkachov]

On 21/02/17 16:58, Matthew Fortune wrote:

Eric Botcazou  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))

[Matthew Fortune]

Eric Botcazou  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))

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

Christophe Lyon  writes:
> On 21 February 2017 at 11:59, Kyrill Tkachov
>  wrote:
> >
> > On 21/02/17 10:54, Christophe Lyon wrote:
> >>
> >> Hi,
> >>
> >> On 20 February 2017 at 13:08, Matthew Fortune
> >>  wrote:
> >>>
> >>> Vladimir Makarov  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*)
> >>
> >> 

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

[Christophe Lyon]

On 21 February 2017 at 11:59, Kyrill Tkachov
 wrote:
>
> On 21/02/17 10:54, Christophe Lyon wrote:
>>
>> Hi,
>>
>> On 20 February 2017 at 13:08, Matthew Fortune
>>  wrote:
>>>
>>> Vladimir Makarov  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))

[Kyrill Tkachov]

On 21/02/17 10:54, Christophe Lyon wrote:

Hi,

On 20 February 2017 at 13:08, Matthew Fortune
 wrote:

Vladimir Makarov  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]

Hi,

On 20 February 2017 at 13:08, Matthew Fortune
 wrote:
> Vladimir Makarov  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))

[Matthew Fortune]

Vladimir Makarov  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))

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

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

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