Need help with PR71976 combine.c::get_last_value returns a wrong result

[Georg-Johann Lay]

On 26.07.2016 14:51, Segher Boessenkool wrote:
> On Tue, Jul 26, 2016 at 02:14:49PM +0200, Georg-Johann Lay wrote:
>>>> which returns const0_rtx because reg 18 is set in insn 43 to const0_rtx.
>>>> Total outcome is that the right shift of reg:DI 18 is transformed to a
>>>> no-op move and cancelled out in the remainder.
>>>
>>> Why does num_sign_bit_copies return something bigger than 8?
>>
>> Because it thinks the last value was const0_rtx (which is incorrect) and
>
> Why is that incorrect?  See insn 43.  Is there another insn I missed?

Yes, the insn stream that matters is (prior to combine):

(insn 43 31 44 2 (set (reg:QI 18 r18)
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 44 43 45 2 (set (reg:QI 19 r19 [+1 ])
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 45 44 46 2 (set (reg:QI 20 r20 [+2 ])
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 46 45 47 2 (set (reg:QI 21 r21 [+3 ])
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 47 46 48 2 (set (reg:QI 22 r22 [+4 ])
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 48 47 49 2 (set (reg:QI 23 r23 [+5 ])
         (reg:QI 65 [ _3+5 ])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 49 48 50 2 (set (reg:QI 24 r24 [+6 ])
         (reg:QI 66 [ _3+6 ])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 50 49 51 2 (set (reg:QI 25 r25 [+7 ])
         (reg:QI 67 [ _3+7 ])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 51 50 52 2 (set (reg:QI 16 r16)
         (const_int 40 [0x28])) bug-combin.c:29 56 {movqi_insn}
      (nil))
(insn 52 51 61 2 (set (reg:DI 18 r18)
         (ashiftrt:DI (reg:DI 18 r18)
             (reg:QI 16 r16))) bug-combin.c:29 1417 {ashrdi3_insn}
      (expr_list:REG_DEAD (reg:QI 16 r16)
         (nil)))

The SETs of insns 43...50 all overlap reg:DI 18.  This is what expand does at 
it breaks up a DImode move into 8 QImode moves.  The first 8 instructions 
comprise an &= 0xffffff0000000000.

It's corect that QI:18 is set to 0, but this does not apply to DI:18 of course.

>> The problem might be that get_last_value() is called for reg:DI 18 and
>> combine's internal bookkeeping is incorrect
>>
>> struct reg_stat_type {
>>   /* Record last point of death of (hard or pseudo) register n.  */
>>   rtx_insn			*last_death;
>>
>>   /* Record last point of modification of (hard or pseudo) register n.  */
>>   rtx_insn			*last_set;
>>
>> .last_set is the set for reg:QI 18 but later insns also change hard reg:DI
>> 18, for example the set of reg:QI 19.  This means that the information in
>> reg_stat_type does not tell the whole story for hard regs.
>>
>> One fix could be to get the mode precision of the SET from the last_set
>> insn and only use the information if that mode is at least as wide as the
>> mode of the register for which get_last_value is called.  reg_stat_type has
>> .last_set_mode for this purpose (QImode in the test case).
>
> Yes, last_set_mode, and there is last_set_sign_bit_copies too.  Are those
> correct?

(gdb) p *rsp
$9 = {last_death = 0x0, last_set = 0x7ffff730cc00, last_set_value = 
0x7ffff73094f0, last_set_table_tick = 2, last_set_label = 2, 
last_set_nonzero_bits = 0, last_set_sign_bit_copies = 8 '\b', last_set_mode = 
QImode, last_set_invalid = 1 '\001', sign_bit_copies = 0 '\000', nonzero_bits = 
0, truncation_label = 2, truncated_to_mode = DImode}

(gdb) p rsp->last_set
$10 = (rtx_insn *) 0x7ffff730cc00
(gdb) pr
(insn 43 31 44 2 (set (reg:QI 18 r18)
         (const_int 0 [0])) bug-combin.c:29 56 {movqi_insn}
      (nil))

(gdb) p rsp->last_set_value
$11 = (rtx) 0x7ffff73094f0
(gdb) pr
(const_int 0 [0])

The .last_set_sign_bit_copies is correct if it is applied to QImode, but the 
sign bits of reg:DI 18 are contained in reg:QI 25 set by insn 50.

As already said, rsp[regno] is not used to get the number of sign bit copies 
but to conclude that DI:18 is entirely set to 0, and the number of sign bit 
copies is then computed for that 0.

>>> We do have a value, and it is for this bb.
>>
>> But not for the complete hard register; it's only for reg:QI 18 which is
>> one byte of reg:DI 18,
>
> The rest of the hard register bits are set to unspecified values.

No, insns 44...50 set the rest of reg:DI 18 to specified values.  This is what 
expand does as is lowers DImode to word_mode (QImode for avr).

FYI, FIRST_PSEUDO_REGISTER is 34.

>> thus the code should never reach this point in the
>> first place and return earlier.  For example:
>>
>> Index: combine.c
>> ===================================================================
>> --- combine.c   (revision 238701)
>> +++ combine.c   (working copy)
>> @@ -13206,6 +13206,13 @@ get_last_value (const_rtx x)
>>        && DF_INSN_LUID (rsp->last_set) >= subst_low_luid)
>>      return 0;
>>
>> +  /* If the lookup is for a hard register make sure that value contains at
>> least
>> +     as many bits as x does.  */
>> +
>> +  if (regno < FIRST_PSEUDO_REGISTER
>> +      && GET_MODE_PRECISION (rsp->last_set_mode) < GET_MODE_PRECISION
>> (GET_MODE (x)))
>> +    return 0;
>> +
>>    /* If the value has all its registers valid, return it.  */
>>    if (get_last_value_validate (&value, rsp->last_set, rsp->last_set_label,
>>    0))
>>      return value;
>
> That might be a bit harsh.

In what respect?

> First things first: is the information recorded correct?

I think yes, but care must be taken what may be concluded from that 
information.  From a set of 8 bits you cannot draw conclusion about all 64 
bits; this should be obvious enough :-)

In the above case rsp[regno] holds only information for 1 sub-byte.  In order 
to get the complete DImode story we would have to get the info for all 
sub-parts and then put them together...

Johann
>
>
> Segher
>