Re: [PATCH] predict: Adjust optimize_function_for_size_p [PR105818]

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

on 2022/8/29 14:35, Kewen.Lin via Gcc-patches wrote:
> on 2022/8/15 16:33, Kewen.Lin via Gcc-patches wrote:
>> on 2022/7/11 11:42, Kewen.Lin wrote:
>>> on 2022/6/15 14:20, Kewen.Lin wrote:
>>>> Hi Honza,
>>>>
>>>> Thanks for the comments!  Some replies are inlined below.
>>>>
>>>> on 2022/6/14 19:37, Jan Hubicka wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Function optimize_function_for_size_p returns OPTIMIZE_SIZE_NO
>>>>>> if func->decl is not null but no cgraph node is available for it.
>>>>>> As PR105818 shows, this could give unexpected result.  For the
>>>>>> case in PR105818, when parsing bar decl in function foo, the cfun
>>>>>> is a function structure for foo, for which there is none cgraph
>>>>>> node, so it returns OPTIMIZE_SIZE_NO.  But it's incorrect since
>>>>>> the context is to optimize for size, the flag optimize_size is
>>>>>> true.
>>>>>>
>>>>>> The patch is to make optimize_function_for_size_p to check
>>>>>> optimize_size as what it does when func->decl is unavailable.
>>>>>>
>>>>>> One regression failure got exposed on aarch64-linux-gnu:
>>>>>>
>>>>>> PASS->FAIL: gcc.dg/guality/pr54693-2.c   -Os \
>>>>>> 	    -DPREVENT_OPTIMIZATION  line 21 x == 10 - i
>>>>>>
>>>>>> The difference comes from the macro LOGICAL_OP_NON_SHORT_CIRCUIT
>>>>>> used in function fold_range_test during c parsing, it uses
>>>>>> optimize_function_for_speed_p which is equal to the invertion
>>>>>> of optimize_function_for_size_p.  At that time cfun->decl is valid
>>>>>> but no cgraph node for it, w/o this patch function
>>>>>> optimize_function_for_speed_p returns true eventually, while it
>>>>>> returns false with this patch.  Since the command line option -Os
>>>>>> is specified, there is no reason to interpret it as "for speed".
>>>>>> I think this failure is expected and adjust the test case
>>>>>> accordingly.
>>>>>>
>>>>>> Is it ok for trunk?
>>>>>>
>>>>>> BR,
>>>>>> Kewen
>>>>>> -----
>>>>>>
>>>>>> 	PR target/105818
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 	* predict.cc (optimize_function_for_size_p): Check optimize_size when
>>>>>> 	func->decl is valid but its cgraph node is unavailable.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 	* gcc.target/powerpc/pr105818.c: New test.
>>>>>> 	* gcc.dg/guality/pr54693-2.c: Adjust for aarch64.
>>>>>> ---
>>>>>>  gcc/predict.cc                              | 2 +-
>>>>>>  gcc/testsuite/gcc.dg/guality/pr54693-2.c    | 2 +-
>>>>>>  gcc/testsuite/gcc.target/powerpc/pr105818.c | 9 +++++++++
>>>>>>  3 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr105818.c
>>>>>>
>>>>>> diff --git a/gcc/predict.cc b/gcc/predict.cc
>>>>>> index 5734e4c8516..6c60a973236 100644
>>>>>> --- a/gcc/predict.cc
>>>>>> +++ b/gcc/predict.cc
>>>>>> @@ -268,7 +268,7 @@ optimize_function_for_size_p (struct function *fun)
>>>>>>    cgraph_node *n = cgraph_node::get (fun->decl);
>>>>>>    if (n)
>>>>>>      return n->optimize_for_size_p ();
>>>>>> -  return OPTIMIZE_SIZE_NO;
>>>>>> +  return optimize_size ? OPTIMIZE_SIZE_MAX : OPTIMIZE_SIZE_NO;
>>>>>
>>>>> We could also do (opt_for_fn (cfun->decl, optimize_size) that is
>>>>> probably better since one can change optimize_size with optimization
>>>>> attribute.
>>>>
>>>> Good point, agree!
>>>>
>>>>> However I think in most cases we check for optimize_size early I think
>>>>> we are doing something wrong, since at that time htere is no profile
>>>>> available.  Why exactly PR105818 hits the flag change issue?
>>>>
>>>> For PR105818, the reason why the flag changs is that:
>>>>
>>>> Firstly, the inconsistent flag is OPTION_MASK_SAVE_TOC_INDIRECT bit
>>>> of rs6000_isa_flags_explicit, it's set as below:
>>>>
>>>> /* 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))
>>>>   rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
>>>>
>>>> Initially, rs6000 initialize target_option_default_node with
>>>> OPTION_MASK_SAVE_TOC_INDIRECT unset, at that time cfun is NULL
>>>> and optimize_size is true.
>>>>
>>>> Later, when c parser handling function foo, it builds target
>>>> option node as target_option_default_node in function
>>>> handle_optimize_attribute, it does global option saving and
>>>> verifying there as well, at that time the cfun is NULL, no
>>>> issue is found.  And function store_parm_decls allocates
>>>> struct_function for foo then, cfun becomes function struct
>>>> for foo, when c parser continues to handle the decl bar in
>>>> foo, function handle_optimize_attribute works as before,
>>>> tries to restore the target options at the end, it calls
>>>> targetm.target_option.restore (rs6000_function_specific_restore)
>>>> which calls function rs6000_option_override_internal again,
>>>> at this time the cfun is not NULL while there is no cgraph
>>>> node for its decl, optimize_function_for_speed_p returns true
>>>> and gets the OPTION_MASK_SAVE_TOC_INDIRECT bit of flag
>>>> rs6000_isa_flags set unexpectedly.  It becomes inconsistent
>>>> as the one saved previously.
>>>>
>>>> IMHO, both contexts of global and function decl foo here hold
>>>> optimize_size, function optimize_function_for_speed_p should
>>>> not return true anyway.
>>>>
>>>> btw, the aarch64 failed case also gets the unexpected
>>>> result for optimize_function_for_speed_p during c parsing
>>>> (fold_range_test <- ... <- c_parser_condition).
>>>>
>>>> IIUC, in parsing time we don't have the profile information
>>>> available.
>>>>
>>>
>>> Hi Honza,
>>>
>>> Does the above explanation sound reasonable to you?
>>>

Hi Honza,

Gentle ping^4 ...

BR,
Kewen