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

[Georg-Johann Lay]

On 25.07.2016 23:05, Segher Boessenkool wrote:
> On Mon, Jul 25, 2016 at 02:28:28PM +0200, Georg-Johann Lay wrote:
>> (insn 43 31 44 2 (set (reg:QI 18 r18)
>>         (const_int 0 [0])) 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)))
>
>>       /* An arithmetic right shift of a quantity known to be -1 or 0
>> 	 is a no-op.  */
>>       if (code == ASHIFTRT
>> 	  && (num_sign_bit_copies (varop, shift_mode)
>> 	      == GET_MODE_PRECISION (shift_mode)))
>> 	{
>> 	  count = 0;
>> 	  break;
>> 	}
>>
>> num_sign_bit_copies() eventually calls
>> combine.c::reg_num_sign_bit_copies_for_combine()
>>
>> 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 hence 
thinks a shift of a reg that's always 0 is the same as a no-op move of the reg 
to itself.

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

>> IIUC get_last_value should return 0 in this case?
>>
>>   /* If we don't have a value, or if it isn't for this basic block and
>>      it's either a hard register, set more than once, or it's a live
>>      at the beginning of the function, return 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, 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;


Johann

>
>
> Segher
>