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

[Segher Boessenkool <segher@kernel.crashing.org>]

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).

> 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?


Segher