Re: PING^1 [PATCH v2] predict: Adjust optimize_function_for_size_p [PR105818]

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

Hi Honza,

Thanks for the comments.

on 2022/12/14 21:22, Jan Hubicka wrote:
>>> 	PR middle-end/105818
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* predict.cc (optimize_function_for_size_p): Further check
>>> 	optimize_size of fun->decl when it is valid but no cgraph node.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* gcc.target/powerpc/pr105818.c: New test.
>>> 	* gcc.dg/guality/pr54693-2.c: Adjust for aarch64.
>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr105818.c b/gcc/testsuite/gcc.target/powerpc/pr105818.c
>>> new file mode 100644
>>> index 00000000000..679647e189d
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr105818.c
>>> @@ -0,0 +1,11 @@
>>> +/* { dg-options "-Os -fno-tree-vectorize" } */
>>> +
>>> +/* Verify there is no ICE.  */
>>> +
>>> +#pragma GCC optimize "-fno-tree-vectorize"
>>> +
>>> +void
>>> +foo (void)
>>> +{
>>> +  void bar (void);
>>> +}
> So the testcase starts with optimize_size set but then it switches to
> optimize_size==0 due to the GCC optimize pragma.  I think this is
> behaviour Martin wants to change, so perhaps the testcase should be
> written with explicit -O2.

No, both the global context and the function context are to optimize
for size, Martin also clarified that.  So the issue is the inconsistent
information on optimizing for size when parsing bar, at that time
cfun->decl is available but no cgraph node, it says OPTIMIZE_SIZE_NO. 

> 
> I also wonder what happen when you add the attribute later?
> /* { dg-options "-Os -fno-tree-vectorize" } */
> 
> /* Verify there is no ICE.  */
> 
> #pragma GCC optimize "-fno-tree-vectorize"
>
// marker A

> void
> foo (void)
> {
>   void bar (void);
> }
> 
> __attribute__ ((optimize("-fno-tree-vectorize"))) void foo (void);

There is still one ICE with this additional decl.  Same ICE if I moved
it to "marker A" above.

> 
> I think we should generally avoid doing decisions about size/speed
> optimizations so early since the setting may change due to attribtes or
> profile feedback...
> 

Good point, I'll make a separated patch to move optimize_function_for_speed_p
uses out of function rs6000_option_override_internal, but I think it's a
separated issue which just results in imperfect "optimize for size" decision.
Fixing it can cover this exposed ICE, but IMHO this exposed inconsistent
information on "optimize for size" exposed here is still an issue, like: all
the context is to optimize for size, but it still returns OPTIMIZE_SIZE_NO.

Do you agree?

BR,
Kewen