Re: [PATCH] ira: Skip some pseudos in move_unallocated_pseudos

[Jeff Law <law@redhat.com>]

On 1/4/21 7:36 PM, Kewen.Lin wrote:
> Hi Jeff,
>
> on 2021/1/5 上午7:13, Jeff Law wrote:
>>
>> On 12/22/20 11:40 PM, Kewen.Lin via Gcc-patches wrote:
>>> Hi Segher,
>>>
>>> on 2020/12/22 下午9:55, Segher Boessenkool wrote:
>>>> Hi!
>>>>
>>>> Just a dumb formatting comment:
>>>>
>>>> On Tue, Dec 22, 2020 at 04:05:39PM +0800, Kewen.Lin wrote:
>>>>> This patch is to make move_unallocated_pseudos consistent
>>>>> to what we have in function find_moveable_pseudos, where we
>>>>> record the original pseudo into pseudo_replaced_reg only if
>>>>> validate_change succeeds with newreg.  To ensure every
>>>>> unallocated pseudo in move_unallocated_pseudos has expected
>>>>> information, it's better to add a check and skip it if it's
>>>>> unexpected.  This avoids possible ICEs in future.
>>>>>
>>>>> btw, I happened to found this in the bootstrapping for one
>>>>> experimental local patch, which is considered as impractical.
>>>>> --- a/gcc/ira.c
>>>>> +++ b/gcc/ira.c
>>>>> @@ -5111,6 +5111,11 @@ move_unallocated_pseudos (void)
>>>>>        {
>>>>>  	int idx = i - first_moveable_pseudo;
>>>>>  	rtx other_reg = pseudo_replaced_reg[idx];
>>>>> +	/* If there is no appropriate pseudo in pseudo_replaced_reg, it
>>>>> +	   means validate_change fails for this new pseudo in function
>>>>> +	   find_moveable_pseudos, then bypass it here.*/
>>>> Dot space space.
>>> Good catch, thanks!  I forgot to reformat after polishing the comments.
>>> Will fix it with other potential comments.
>>>
>>>> The patch sounds fine to me.  Hard to tell without seeing the patch that
>>>> exposed the problem (for onlookers like me who do not know this code
>>>> well, anyway ;-) )
>>> The patch which made this issue exposed looks like:
>>>
>>> +; Like *rotl<mode>3_insert_3 but work with nonzero_bits rather than
>>> +; explicit AND.
>>> +(define_insn "*rotl<mode>3_insert_8"
>>> +  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
>>> +        (ior:GPR (ashift:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
>>> +                             (match_operand:SI 2 "u6bit_cint_operand" "n"))
>>> +                 (match_operand:GPR 3 "gpc_reg_operand" "0")))]
>>> +  "HOST_WIDE_INT_1U << INTVAL (operands[2])
>>> +   > nonzero_bits (operands[3], <MODE>mode)"
>>> +{
>>> +  if (<MODE>mode == SImode)
>>> +    return "rlwimi %0,%1,%h2,0,31-%h2";
>>> +  else
>>> +    return "rldimi %0,%1,%H2,0";
>>> +}
>>> +  [(set_attr "type" "insert")])
>>>
>>> Some insn matches this pattern in combine, later ira tries to introduce
>>> one new pseudo since it meets the checks in find_moveable_pseudos, but
>>> it fails in the call to validate_change since the nonzero_bits is more
>>> rough and can't satisfy the pattern condition, leaving the unexpected
>>> entry in pseudo_replaced_reg.
>> But what doesn't make any sense to me is pseudo_replaced_reg[] is only
>> set when validation is successful in find_moveable_pseudos.   So I can't
>> see how this patch actually helps the problem you're describing.
>>
> Yeah, pseudo_replaced_reg[] is only set when validation is successful,
> but we bump the max pseudo number in ira_create_new_reg as below
> regardless of whether validation succeeds or not:
>
> 	  rtx newreg = ira_create_new_reg (def_reg);
> 	  if (validate_change (def_insn, DF_REF_REAL_LOC (def), newreg, 0))
>
> Later in move_unallocated_pseudos, the iterating could cover those
> pseudos which were created but not used due to failed validation.
>
>   for (i = first_moveable_pseudo; i < last_moveable_pseudo; i++)
>     if (reg_renumber[i] < 0)
>       {
> 	int idx = i - first_moveable_pseudo;
> 	rtx other_reg = pseudo_replaced_reg[idx];                // (1)
> 	rtx_insn *def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (i));
> 	/* The use must follow all definitions of OTHER_REG, so we can
> 	   insert the new definition immediately after any of them.  */
> 	df_ref other_def = DF_REG_DEF_CHAIN (REGNO (other_reg))
>
> Then we can get the NULL other_reg in (1), also have unexpected df info
> which causes ICE.  The patch skips the handlings on those pseudos which
> were intended to be used in validatation INSN but failed to.
I was wondering if it was somehow related to creation of new pseudos. 
The other important tidbit here is we reset last_movable_pseudo near the
end of find_moveable_pseudos.

OK for the trunk with an expanded comment.

Thanks,
jeff