* PR84229 part 1: Avoid cloning of functions that calls va_arg_pack
@ 2018-02-21 19:09 Jan Hubicka
2018-02-21 22:49 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Jan Hubicka @ 2018-02-21 19:09 UTC (permalink / raw)
To: gcc-patches
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.
Index: ipa-cp.c
===================================================================
--- 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
+ becuase ipa-cp will not give up on cloning further. If the function is
+ external this however leads to wrong code becuase we may end up producing
+ 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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PR84229 part 1: Avoid cloning of functions that calls va_arg_pack
2018-02-21 22:49 ` Jakub Jelinek
@ 2018-02-21 20:12 ` Jan Hubicka
0 siblings, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2018-02-21 20:12 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PR84229 part 1: Avoid cloning of functions that calls va_arg_pack
2018-02-21 19:09 PR84229 part 1: Avoid cloning of functions that calls va_arg_pack Jan Hubicka
@ 2018-02-21 22:49 ` Jakub Jelinek
2018-02-21 20:12 ` Jan Hubicka
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2018-02-21 22:49 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-21 22:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-21 19:09 PR84229 part 1: Avoid cloning of functions that calls va_arg_pack Jan Hubicka
2018-02-21 22:49 ` Jakub Jelinek
2018-02-21 20:12 ` Jan Hubicka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).