[RFA,PR,tree-optimization/59749] Fix recently introduced ree bug

On 01/16/14 04:52, Eric Botcazou wrote:
>> Yes, like in the attached patch?  OK for the trunk?
>
> Unfortunately this broke again bootstrap with RTL checking enabled on x86-64:
>
> /home/eric/svn/gcc/libgcc/libgcc2.c: In function '__negdi2':
> /home/eric/svn/gcc/libgcc/libgcc2.c:71:1: internal compiler error: RTL check:
> expected code 'reg', have 'ne' in rhs_regno, at rtl.h:1125
Here's what I'm checking in.  No idea why I was looking at orig_src, 
that's just wrong.  We want to compare # hard regs for the original 
destination's regno/mode vs the # hard regs for the new destination's 
regno/mode.

Bootstrapped & regression tested on x86_64-unknown-linux.  Also 
bootstrapped with --enable-checking=rtl.

Installed on the trunk.
commit e9bbaae3fe335b8eef7022f684bc7c35926471ca
Author: Jeff Law <law@redhat.com>
Date:   Thu Jan 16 14:23:15 2014 -0700

           * ree.c (combine_set_extension): Correct test for changing number
            of hard registers when widening a reaching definition.

On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote:
> +2014-01-16  Jeff Law  <law@redhat.com>
> +
> +	* ree.c (combine_set_extension): Correct test for changing number
> +	of hard registers when widening a reaching definition.
> +
>  2014-01-16  Bernd Schmidt  <bernds@codesourcery.com>
>  
>  	PR middle-end/56791
> diff --git a/gcc/ree.c b/gcc/ree.c
> index 19d821c..96cddd2 100644
> --- a/gcc/ree.c
> +++ b/gcc/ree.c
> @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set)
>    /* We're going to be widening the result of DEF_INSN, ensure that doing so
>       doesn't change the number of hard registers needed for the result.  */
>    if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
> -      != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set))))
> +      != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
> +			   GET_MODE (SET_DEST (*orig_set))))

Shouldn't that be:
    if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
	!= HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set))))
instead?

I mean, for the !copy_needed case it is obviously the same thing (and that
is what triggers in the testcase), but don't we generally want to check if
the same hard register in a wider mode will not occupy more registers, and
in particular the hard register we are considering to use on the lhs of the
defining insn (i.e. new_reg)?

Of course usually HARD_REGNO_NREGS will be the same for the same mode and
different GPR, so it would be really hard to create a testcase.

	Jakub

> Bootstrapped & regression tested on x86_64-unknown-linux.  Also
> bootstrapped with --enable-checking=rtl.

Note that you can do only one bootstrap with --enable-checking=yes,rtl.

> Installed on the trunk.

As far as I can see, no, it was not installed.

On 01/17/14 01:41, Eric Botcazou wrote:
>> Bootstrapped & regression tested on x86_64-unknown-linux.  Also
>> bootstrapped with --enable-checking=rtl.
>
> Note that you can do only one bootstrap with --enable-checking=yes,rtl.
>
>> Installed on the trunk.
>
> As far as I can see, no, it was not installed.
Pilot error, it's not installed.

jefff

On 01/16/14 15:07, Jakub Jelinek wrote:
> On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote:
>> +2014-01-16  Jeff Law  <law@redhat.com>
>> +
>> +	* ree.c (combine_set_extension): Correct test for changing number
>> +	of hard registers when widening a reaching definition.
>> +
>>   2014-01-16  Bernd Schmidt  <bernds@codesourcery.com>
>>
>>   	PR middle-end/56791
>> diff --git a/gcc/ree.c b/gcc/ree.c
>> index 19d821c..96cddd2 100644
>> --- a/gcc/ree.c
>> +++ b/gcc/ree.c
>> @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set)
>>     /* We're going to be widening the result of DEF_INSN, ensure that doing so
>>        doesn't change the number of hard registers needed for the result.  */
>>     if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
>> -      != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set))))
>> +      != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
>> +			   GET_MODE (SET_DEST (*orig_set))))
>
> Shouldn't that be:
>      if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
> 	!= HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set))))
> instead?
>
> I mean, for the !copy_needed case it is obviously the same thing (and that
> is what triggers in the testcase), but don't we generally want to check if
> the same hard register in a wider mode will not occupy more registers, and
> in particular the hard register we are considering to use on the lhs of the
> defining insn (i.e. new_reg)?
I thought about using that conditional more than once.  But talked 
myself out of it every time on the grounds that I wanted to test the 
original destination REGNO of the reaching def.

Obviously that is REGNO (new_reg) if !copy_needed.  But it's something 
completely different if copy_needed.


In the copy_needed case there's actually two destinations to consider. 
  The original destination as well as the new destination.  Both will be 
set in a mode wider than the destination of the original reaching def. 
(one will be set in the modified reaching def and the other in a copy insn).

ISTM we need the # hard reg checked on the original destination as the 
other (upper) hard regs might be live across the sequence, but not 
used/set in the sequence.   Then we need some kind of check on the upper 
part of the new destination...  But I thought I covered that elsewhere...

Anyway, I clearly need to rethink that test.  Given this is something we 
haven't seen in the wild, I'm going to disable it over the 
weekend/monday so that enable-checking bugs pass and continue to ponder.

jeff