PR84229 part 1: Avoid cloning of functions that calls va_arg_pack

Hi,
this fixes first part of the issue in PR84229. The actual testcase dies because
ipa-cp decides to cone function calling va_arg_pack which is later not inlined.
Such functions works only when they are inlined and thus it makes no sense to
inline them.  I disabled cloning only for external function (where this can
make us refuse valid code) becuase otherwise we could prevent some useful
cloning which depends on this.

The underlying issue is deeper. This bug triggers while building firefox
with LTO and -O3 on trunk and GCC 7 branch as well. It is fortified implementation
of open which for some reason is not alwyas_inline in libc headers and we decide
to not early inline it as it looks large (it has a lot of logic that will be optimized
out after inlining).

I think we miss fortification diagnostics this way. I have some followup patches
that makes the function body look smaller but not small enough for early inlining.
I do not see how this is supposed to work well without always_inline attribute.

Bootstrapped/regtested x86_64-linux, comitted.

Honza

	PR c/84229
	* ipa-cp.c (determine_versionability): Do not version functions caling
	va_arg_pack.

On Wed, Feb 21, 2018 at 08:09:28PM +0100, Jan Hubicka wrote:
> --- ipa-cp.c	(revision 257844)
> +++ ipa-cp.c	(working copy)
> @@ -630,6 +630,24 @@ determine_versionability (struct cgraph_
>        reason = "calls comdat-local function";
>      }
>  
> +  /* Functions calling BUILT_IN_VA_ARG_PACK and BUILT_IN_VA_ARG_PACK_LEN
> +     works only when inlined.  Cloning them may still lead to better code

s/works/work/

> +     becuase ipa-cp will not give up on cloning further.  If the function is

s/becuase/because/

> +     external this however leads to wrong code becuase we may end up producing

s/becuase/because/

> +     offline copy of the function.  */
> +  if (DECL_EXTERNAL (node->decl))
> +    for (cgraph_edge *edge = node->callees; !reason && edge;
> +	 edge = edge->next_callee)
> +      if (DECL_BUILT_IN (edge->callee->decl)
> +	  && DECL_BUILT_IN_CLASS (edge->callee->decl) == BUILT_IN_NORMAL)
> +        {
> +	  if (DECL_FUNCTION_CODE (edge->callee->decl) == BUILT_IN_VA_ARG_PACK)
> +	    reason = "external function which calls va_arg_pack";
> +	  if (DECL_FUNCTION_CODE (edge->callee->decl)
> +	      == BUILT_IN_VA_ARG_PACK_LEN)
> +	    reason = "external function which calls va_arg_pack_len";
> +        }
> +
>    if (reason && dump_file && !node->alias && !node->thunk.thunk_p)
>      fprintf (dump_file, "Function %s is not versionable, reason: %s.\n",
>  	     node->dump_name (), reason);

Do you have a testcase for this, or is it LTO with too large input?

	Jakub

> On Wed, Feb 21, 2018 at 08:09:28PM +0100, Jan Hubicka wrote:
> > --- ipa-cp.c	(revision 257844)
> > +++ ipa-cp.c	(working copy)
> > @@ -630,6 +630,24 @@ determine_versionability (struct cgraph_
> >        reason = "calls comdat-local function";
> >      }
> >  
> > +  /* Functions calling BUILT_IN_VA_ARG_PACK and BUILT_IN_VA_ARG_PACK_LEN
> > +     works only when inlined.  Cloning them may still lead to better code
> 
> s/works/work/
> 
> > +     becuase ipa-cp will not give up on cloning further.  If the function is
> 
> s/becuase/because/
> 
> > +     external this however leads to wrong code becuase we may end up producing
> 
> s/becuase/because/

Thanks, fixed.

> 
> > +     offline copy of the function.  */
> > +  if (DECL_EXTERNAL (node->decl))
> > +    for (cgraph_edge *edge = node->callees; !reason && edge;
> > +	 edge = edge->next_callee)
> > +      if (DECL_BUILT_IN (edge->callee->decl)
> > +	  && DECL_BUILT_IN_CLASS (edge->callee->decl) == BUILT_IN_NORMAL)
> > +        {
> > +	  if (DECL_FUNCTION_CODE (edge->callee->decl) == BUILT_IN_VA_ARG_PACK)
> > +	    reason = "external function which calls va_arg_pack";
> > +	  if (DECL_FUNCTION_CODE (edge->callee->decl)
> > +	      == BUILT_IN_VA_ARG_PACK_LEN)
> > +	    reason = "external function which calls va_arg_pack_len";
> > +        }
> > +
> >    if (reason && dump_file && !node->alias && !node->thunk.thunk_p)
> >      fprintf (dump_file, "Function %s is not versionable, reason: %s.\n",
> >  	     node->dump_name (), reason);
> 
> Do you have a testcase for this, or is it LTO with too large input?

There is reduced testcase in the PR which probably should be rejected by C
frontend because it is uses va_arg_pack on non-variadic function.
I plan to send additional patches and then come with a testcase (because current
one should be early inlined and thus it will stop testing ipa-cp)

Honza