Re: [PATCH] rtl-optimization/110587 - speedup find_hard_regno_for_1

[Jeff Law <jeffreyalaw@gmail.com>]

On 8/8/23 10:49, Vladimir Makarov wrote:
> 
> On 8/7/23 09:18, Richard Biener wrote:
>> On Wed, 2 Aug 2023, Richard Biener wrote:
>>
>>> On Mon, 31 Jul 2023, Jeff Law wrote:
>>>
>>>>
>>>> On 7/31/23 04:54, Richard Biener via Gcc-patches wrote:
>>>>> On Tue, 25 Jul 2023, Richard Biener wrote:
>>>>>
>>>>>> The following applies a micro-optimization to find_hard_regno_for_1,
>>>>>> re-ordering the check so we can easily jump-thread by using an else.
>>>>>> This reduces the time spent in this function by 15% for the testcase
>>>>>> in the PR.
>>>>>>
>>>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK if that
>>>>>> passes?
>>>>> Ping.
>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>   PR rtl-optimization/110587
>>>>>>   * lra-assigns.cc (find_hard_regno_for_1): Re-order checks.
>>>>>> ---
>>>>>>    gcc/lra-assigns.cc | 9 +++++----
>>>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
>>>>>> index b8582dcafff..d2ebcfd5056 100644
>>>>>> --- a/gcc/lra-assigns.cc
>>>>>> +++ b/gcc/lra-assigns.cc
>>>>>> @@ -522,14 +522,15 @@ find_hard_regno_for_1 (int regno, int *cost, 
>>>>>> int
>>>>>> @@ try_only_hard_regno,
>>>>>>            r2 != NULL;
>>>>>>            r2 = r2->start_next)
>>>>>>            {
>>>>>> -          if (r2->regno >= lra_constraint_new_regno_start
>>>>>> +          if (live_pseudos_reg_renumber[r2->regno] < 0
>>>>>> +          && r2->regno >= lra_constraint_new_regno_start
>>>>>>        && lra_reg_info[r2->regno].preferred_hard_regno1 >= 0
>>>>>> -          && live_pseudos_reg_renumber[r2->regno] < 0
>>>>>>        && rclass_intersect_p[regno_allocno_class_array[r2->regno]])
>>>>>>      sparseset_set_bit (conflict_reload_and_inheritance_pseudos,
>>>>>>                       r2->regno);
>>>>>> -          if (live_pseudos_reg_renumber[r2->regno] >= 0
>>>>>> -          && 
>>>>>> rclass_intersect_p[regno_allocno_class_array[r2->regno]])
>>>>>> +          else if (live_pseudos_reg_renumber[r2->regno] >= 0
>>>>>> +               && rclass_intersect_p
>>>>>> +                [regno_allocno_class_array[r2->regno]])
>>>>>>      sparseset_set_bit (live_range_hard_reg_pseudos, r2->regno);
>>>> My biggest concern here would be r2->regno < 0  in the new code 
>>>> which could
>>>> cause an OOB array reference in the first condition of the test.
>>>>
>>>> Isn't that the point if the original ordering?  Test that r2->regno is
>>>> reasonable before using it as an array index?
>>> Note the original code is
>>>
>>>                if (r2->regno >= lra_constraint_new_regno_start
>>> ...
>>>           if (live_pseudos_reg_renumber[r2->regno] >= 0
>>> ...
>>>
>>> so we are going to access live_pseudos_reg_renumber[r2->regno]
>>> independent on the r2->regno >= lra_constraint_new_regno_start check,
>>> so I don't think that's the point of the original ordering.  Note
>>> I preserved the ordering with respect to other array accesses,
>>> the speedup seen is because we now have the
>>>
>>>
>>>     if (live_pseudos_reg_renumber[r2->regno] < 0
>>>         ...
>>>     else if (live_pseudos_reg_renumber[r2->regno] >= 0
>>>              ...
>>>
>>> structure directly exposed which helps the compiler.
>>>
>>> I think the check on r2->regno is to decide whether to alter
>>> conflict_reload_and_inheritance_pseudos or
>>> live_range_hard_reg_pseudos (so it's also somewhat natural to check
>>> that first).
>> So - OK?
>>
> Richard, sorry, I overlooked this thread.
> 
> Yes, it is OK to commit.  In general Jeff has a reasonable concern but 
> in this case r2->regno is always >= 0 and I can not imagine reasons that 
> we will change algorithm in the future in such way when it is not true.
Thanks for confirming it's a non-issue.  No objection from me.

jeff