Re: [PATCH] Attribute noclone

[Richard Guenther <rguenther at suse dot de>]

On Fri, 24 Jul 2009, Martin Jambor wrote:

> Hi,
> 
> On Fri, Jul 17, 2009 at 09:30:11PM +0200, Martin Jambor wrote:
> > Hi,
> > 
> > since the  attribute noinline does not prevent  ipa-cp cloning from
> > taking place, there  has been a need to provide  a new attribute to
> > do just that.   The following patch introduces it,  adds a testcase
> > to verify  it works, amends  a testcase which  will need it  in the
> > future and documents it.
> > 
> 
> I have talked about this with Honza and we have come to the conclusion
> that inlining and cloning  are substantially different and users might
> want   to  preclude   inlining   while  allowing   for  various   code
> specializations and  techniques that copy function bodies  and thus we
> would both rather see noclone as a separate attribute.

Users should use noinline if inlining a function would violate constraints
they set.  I can't see what constraints would not be violated by cloning
at the same time.  Also we break existing code that follows our own
manuals advice if we only disable inlining with the noinline attribute
but create clones.  To cite the manual:

"The @code{&&foo} expressions for the same label might have different 
values
if the containing function is inlined or cloned.  If a program relies on
them being always the same, @code{__attribute__((__noinline__))} should
be used to prevent inlining."

Most users will know about inlining but I suspect very few have the
idea of the compiler cloning their functions.

"@deftypefn {Built-in Function} {void *} __builtin_return_address 
(unsigned int @var{level})
...
When inlining
the expected behavior is that the function will return the address of
the function that will be returned to.  To work around this behavior use
the @code{noinline} function attribute."

So I still think that noinline should disable all function cloning.

Richard.

> The previous patch was indeed too ipa-cp centric, this one is
> hopefully better in this regard, checking the flag in
> tree_versionable_function_p in tree-inline.c instead.
> 
> Bootstrapped and tested on x86_64-linux.  OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2009-07-23  Martin Jambor  <mjambor@suse.cz>
> 
> 	* doc/extend.texi (Labels as Values): Document need for noclone.
> 	(Function Attributes): Document noclone attribute.
> 	(Return Address): Mention potential need for the caller a function
> 	using __builtin_return_address to have noinline and noclone
> 	attributes .
> 	* c-common.c (c_common_attribute_table): New element for noclone.
> 	(handle_noclone_attribute): New function. Forward-declare.
> 	* tree-inline.c (tree_versionable_function_p): Check for noclone
> 	attribute.
> 
> 	* testsuite/gcc.c-torture/execute/pr17377.c: Add noclone attribute to
> 	function y.
> 	* testsuite/gcc.dg/ipa/noclone-1.c: New test.
> 
> 
> Index: icln/gcc/c-common.c
> ===================================================================
> --- icln.orig/gcc/c-common.c	2009-07-24 01:09:02.000000000 +0200
> +++ icln/gcc/c-common.c	2009-07-24 01:16:08.000000000 +0200
> @@ -482,6 +482,7 @@ static tree handle_noreturn_attribute (t
>  static tree handle_hot_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_cold_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_always_inline_attribute (tree *, tree, tree, int,
>  					    bool *);
>  static tree handle_gnu_inline_attribute (tree *, tree, tree, int, bool *);
> @@ -733,6 +734,8 @@ const struct attribute_spec c_common_att
>  			      handle_noreturn_attribute },
>    { "noinline",               0, 0, true,  false, false,
>  			      handle_noinline_attribute },
> +  { "noclone",                0, 0, true,  false, false,
> +			      handle_noclone_attribute },
>    { "always_inline",          0, 0, true,  false, false,
>  			      handle_always_inline_attribute },
>    { "gnu_inline",             0, 0, true,  false, false,
> @@ -5913,6 +5916,23 @@ handle_noinline_attribute (tree *node, t
>    return NULL_TREE;
>  }
>  
> +/* Handle a "noclone" attribute; arguments as in
> +   struct attribute_spec.handler.  */
> +
> +static tree
> +handle_noclone_attribute (tree *node, tree name,
> +			  tree ARG_UNUSED (args),
> +			  int ARG_UNUSED (flags), bool *no_add_attrs)
> +{
> +  if (TREE_CODE (*node) != FUNCTION_DECL)
> +    {
> +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> +      *no_add_attrs = true;
> +    }
> +
> +  return NULL_TREE;
> +}
> +
>  /* Handle a "always_inline" attribute; arguments as in
>     struct attribute_spec.handler.  */
>  
> Index: icln/gcc/doc/extend.texi
> ===================================================================
> --- icln.orig/gcc/doc/extend.texi	2009-07-24 01:09:02.000000000 +0200
> +++ icln/gcc/doc/extend.texi	2009-07-24 01:16:08.000000000 +0200
> @@ -372,11 +372,12 @@ This is more friendly to code living in 
>  the number of dynamic relocations that are needed, and by consequence,
>  allows the data to be read-only.
>  
> -The @code{&&foo} expressions for the same label might have different values
> -if the containing function is inlined or cloned.  If a program relies on
> -them being always the same, @code{__attribute__((__noinline__))} should
> -be used to prevent inlining.  If @code{&&foo} is used
> -in a static variable initializer, inlining is forbidden.
> +The @code{&&foo} expressions for the same label might have different
> +values if the containing function is inlined or cloned.  If a program
> +relies on them being always the same,
> +@code{__attribute__((__noinline__,__noclone__))} should be used to
> +prevent inlining and cloning.  If @code{&&foo} is used in a static
> +variable initializer, inlining and cloning is forbidden.
>  
>  @node Nested Functions
>  @section Nested Functions
> @@ -1881,19 +1882,18 @@ attributes when making a declaration.  T
>  attribute specification inside double parentheses.  The following
>  attributes are currently defined for functions on all targets:
>  @code{aligned}, @code{alloc_size}, @code{noreturn},
> -@code{returns_twice}, @code{noinline}, @code{always_inline},
> -@code{flatten}, @code{pure}, @code{const}, @code{nothrow},
> -@code{sentinel}, @code{format}, @code{format_arg},
> +@code{returns_twice}, @code{noinline}, @code{noclone},
> +@code{always_inline}, @code{flatten}, @code{pure}, @code{const},
> +@code{nothrow}, @code{sentinel}, @code{format}, @code{format_arg},
>  @code{no_instrument_function}, @code{section}, @code{constructor},
>  @code{destructor}, @code{used}, @code{unused}, @code{deprecated},
>  @code{weak}, @code{malloc}, @code{alias}, @code{warn_unused_result},
>  @code{nonnull}, @code{gnu_inline}, @code{externally_visible},
> -@code{hot}, @code{cold}, @code{artificial}, @code{error}
> -and @code{warning}.
> -Several other attributes are defined for functions on particular
> -target systems.  Other attributes, including @code{section} are
> -supported for variables declarations (@pxref{Variable Attributes}) and
> -for types (@pxref{Type Attributes}).
> +@code{hot}, @code{cold}, @code{artificial}, @code{error} and
> +@code{warning}.  Several other attributes are defined for functions on
> +particular target systems.  Other attributes, including @code{section}
> +are supported for variables declarations (@pxref{Variable Attributes})
> +and for types (@pxref{Type Attributes}).
>  
>  You may also specify attributes with @samp{__} preceding and following
>  each keyword.  This allows you to use them in header files without
> @@ -2718,6 +2718,13 @@ asm ("");
>  (@pxref{Extended Asm}) in the called function, to serve as a special
>  side-effect.
>  
> +@item noclone
> +@cindex @code{noclone} function attribute
> +This function attribute prevents a function from being considered for
> +cloning - a mechanism which produces specialized copies of functions
> +and which is (currently) performed by interprocedural constant
> +propagation.
> +
>  @item nonnull (@var{arg-index}, @dots{})
>  @cindex @code{nonnull} function attribute
>  The @code{nonnull} attribute specifies that some function parameters should
> @@ -5798,14 +5805,18 @@ These functions may be used to get infor
>  function.
>  
>  @deftypefn {Built-in Function} {void *} __builtin_return_address (unsigned int @var{level})
> -This function returns the return address of the current function, or of
> -one of its callers.  The @var{level} argument is number of frames to
> -scan up the call stack.  A value of @code{0} yields the return address
> -of the current function, a value of @code{1} yields the return address
> -of the caller of the current function, and so forth.  When inlining
> -the expected behavior is that the function will return the address of
> -the function that will be returned to.  To work around this behavior use
> -the @code{noinline} function attribute.
> +This function returns the return address of the current function, or
> +of one of its callers.  The @var{level} argument is number of frames
> +to scan up the call stack.  A value of @code{0} yields the return
> +address of the current function, a value of @code{1} yields the return
> +address of the caller of the current function, and so forth.  When
> +inlining the expected behavior is that the function will return the
> +address of the function that will be returned to.  To work around this
> +behavior use the @code{noinline} function attribute.  When a caller of
> +a function calling this built-in has been inlined or cloned, the
> +address might differ even when this is not apprent from the source
> +code.  To prevent this from happening, use both the @code{noinline}
> +and @code{noclone} function attributes on the caller.
>  
>  The @var{level} argument must be a constant integer.
>  
> Index: icln/gcc/testsuite/gcc.c-torture/execute/pr17377.c
> ===================================================================
> --- icln.orig/gcc/testsuite/gcc.c-torture/execute/pr17377.c	2009-07-24 01:09:02.000000000 +0200
> +++ icln/gcc/testsuite/gcc.c-torture/execute/pr17377.c	2009-07-24 01:16:08.000000000 +0200
> @@ -26,7 +26,7 @@ f (int i)
>  
>  int x;
>  
> -void *y (int i) __attribute__ ((__noinline__));
> +void *y (int i) __attribute__ ((__noinline__,__noclone__));
>  void *
>  y (int i)
>  {
> Index: icln/gcc/testsuite/gcc.dg/ipa/noclone-1.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ icln/gcc/testsuite/gcc.dg/ipa/noclone-1.c	2009-07-24 01:16:08.000000000 +0200
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp -fno-early-inlining"  } */
> +
> +int global_1, global_2;
> +
> +__attribute__((__noclone__)) int g (int b, int c)
> + {
> +  global_1 = b;
> +  global_2 = c;
> +}
> +
> +__attribute__((__noclone__)) int f (int a)
> +{
> +  /* Second parameter of g gets different values.  */
> +  if (a > 0)
> +    g (a, 3);
> +  else
> +    g (a, 5);
> +}
> +
> +int main ()
> +{
> +  f (7);
> +  return 0;
> +}
> +
> +
> +/* { dg-final { scan-ipa-dump-times "versioned function" 0 "cp"  } } */
> +/* { dg-final { cleanup-ipa-dump "cp" } } */
> Index: icln/gcc/tree-inline.c
> ===================================================================
> --- icln.orig/gcc/tree-inline.c	2009-07-24 01:09:02.000000000 +0200
> +++ icln/gcc/tree-inline.c	2009-07-24 01:16:08.000000000 +0200
> @@ -4349,7 +4349,8 @@ copy_static_chain (tree static_chain, co
>  bool
>  tree_versionable_function_p (tree fndecl)
>  {
> -  return copy_forbidden (DECL_STRUCT_FUNCTION (fndecl), fndecl) == NULL;
> +  return (copy_forbidden (DECL_STRUCT_FUNCTION (fndecl), fndecl) == NULL)
> +    && !lookup_attribute ("noclone", DECL_ATTRIBUTES (fndecl));
>  }
>  
>  /* Delete all unreachable basic blocks and update callgraph.
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex