Re: [PATCH] rs6000: Don't use optimize_function_for_speed_p too early [PR108184]

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2023/1/4 22:02, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Jan 04, 2023 at 08:15:03PM +0800, Kewen.Lin wrote:
>> on 2023/1/4 18:46, Segher Boessenkool wrote:
>>>> @@ -25604,7 +25602,9 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>>>>
>>>>  	  /* Can we optimize saving the TOC in the prologue or
>>>>  	     do we need to do it at every call?  */
>>>> -	  if (TARGET_SAVE_TOC_INDIRECT && !cfun->calls_alloca)
>>>> +	  if (TARGET_SAVE_TOC_INDIRECT
>>>> +	      && !cfun->calls_alloca
>>>> +	      && optimize_function_for_speed_p (cfun))
>>>>  	    cfun->machine->save_toc_in_prologue = true;
>>>
>>> Is this correct?  If so, it really needs a separate testcase.
>>
>> Yes, it just moves the condition from:
>>
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -3978,8 +3978,7 @@ rs6000_option_override_internal (bool global_init_p)
>>    /* If we can shrink-wrap the TOC register save separately, then use
>>       -msave-toc-indirect unless explicitly disabled.  */
>>    if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
>> -      && flag_shrink_wrap_separate
>> -      && optimize_function_for_speed_p (cfun))
>> +      && flag_shrink_wrap_separate)
>>      rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
>>
>> here.
> 
> That "just" reinforces that this really needs a testcase!  It is all
> action at a distance, none of this is trivial (if it was there would
> not be a bug here in the first place, of course).

OK, I'll make a test case for it. :)

> 
>> I tried to find one test case before, but failed to find one which is not fragile
>> to test.  And I thought the associated test case has demonstrated why the use of
>> optimize_function_for_{speed,size}_p is too early in function
>> rs6000_option_override_internal, so I gave up then.  Do you worry about that we
>> could revert it unexpectedly in future and no sensitive test case is on it?
> 
> I worry that it might contradict what some other code does.  I also
> worry that it just is not a sensible thing to do.
> 
> I do not worry that your patch is not an improvement.  But the resulting
> code more clearly (than the original) is problematic.  Where is r2 saved
> to the frame if save_toc_in_prologue is false?

If save_toc_in_prologue is false, the r2 saving to frame would occur at each
indirect call.  Currently separate shrink-wrapping will check save_toc_in_prologue
to decide whether to consider saving toc as one component, I think that's why
we enable save-toc-indirect implicitly (going to set save_toc_in_prologue)
if it's not specified explicitly and doing separate shrink-wrapping.

BR,
Kewen