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
