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
