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
diff --git a/gcc/ira.c b/gcc/ira.c
index 89b5df4003d..58c1efe54b5 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5111,6 +5111,15 @@ move_unallocated_pseudos (void)
{
int idx = i - first_moveable_pseudo;
rtx other_reg = pseudo_replaced_reg[idx];
+ /* The iterating range [first_moveable_pseudo, last_moveable_pseudo)
+ covers every new pseudo created in find_moveable_pseudos,
+ regardless of the validation with it is successful or not.
+ So we need to skip the pseudos which were used in those failed
+ validations to avoid unexpected DF info and consequent ICE.
+ We only set pseudo_replaced_reg[] when the validation is successful
+ in find_moveable_pseudos, it's enough to check it here. */
+ if (!other_reg)
+ continue;
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. */
