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