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

[Jeff Law <law@redhat.com>]

On 1/5/21 8:12 PM, Kewen.Lin wrote:
> on 2021/1/6 上午2:19, Jeff Law wrote:
>>
>> 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.
> Yeah, the iterating will scan all new pseudos created in find_moveable_pseudos,
> the problem occurs on those ones that fail to validate.
>
>> OK for the trunk with an expanded comment.
> Thanks!  Does the attached new version look good to you?
Yes.  Thanks.
jeff