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?

BR,
Kewen