Hello.

----- Original Message ----- 
From: "Bernd Schmidt" <bschmidt@redhat.com>
Sent: Tuesday, October 27, 2015 1:50 PM

> On 10/26/2015 11:46 PM, Anatoliy Sokolov wrote:
>>    This patch change code 'REGNO (subreg) + subreg_regno_offset (...)'
>> with subreg_regno (subreg).
>
>
>> Index: gcc/reg-stack.c
>> ===================================================================
>> --- gcc/reg-stack.c    (revision 229083)
>> +++ gcc/reg-stack.c    (working copy)
>> @@ -416,11 +416,7 @@
>>         rtx subreg;
>>         if (STACK_REG_P (subreg = SUBREG_REG (*pat)))
>>           {
>
> Isn't this wrong? subreg_regno wants to be called with a SUBREG, but here 
> we already had subreg = SUBREG_REG (*pat).
>

Fixed.

>> @@ -5522,12 +5516,7 @@
>>           op0 = SUBREG_REG (op0);
>>           code0 = GET_CODE (op0);
>>           if (code0 == REG && REGNO (op0) < FIRST_PSEUDO_REGISTER)
>> -          op0 = gen_rtx_REG (word_mode,
>> -                 (REGNO (op0) +
>> -                  subreg_regno_offset (REGNO (SUBREG_REG (orig_op0)),
>> -                               GET_MODE (SUBREG_REG (orig_op0)),
>> -                               SUBREG_BYTE (orig_op0),
>> -                               GET_MODE (orig_op0))));
>> +          op0 = gen_rtx_REG (word_mode, subreg_regno (op0));
>>         }
>
> Same problem as in the reg-stack code? The existing code was using 
> orig_op0 to get the subreg, you've changed it to use op0 which is already 
> the SUBREG_REG.
>

No promblens here. At this point op0 is equivalent orig_op0. New value to 
op0 can be assigned later.

> With an x86 test you're not exercising reload, and even on other targets 
> this is not a frequently used path. I've stopped reviewing here, I think 
> this is a good example of the kind of cleanup patch we _shouldn't_ be 
> doing. We've proved it's risky, and unless these cleanup patches were a 
> preparation for functional changes, we should just leave the code alone 
> IMO.
>
>
> Bernd

Ok. In any case, a revised patch.

Anatoly.