On Thu, 1 Oct 2020, Jan Hubicka wrote:
> Hi
> > > + if (!fnspec.arg_specified_p (arg))
> > > + ;
> > > + else if (!fnspec.arg_used_p (arg))
> > > + flags = EAF_UNUSED;
> > > + else
> > > + {
> > > + if (!fnspec.arg_direct_p (arg))
> >
> > negated test
> >
> > > + flags |= EAF_DIRECT;
> > > + if (!fnspec.arg_noescape_p (arg))
> > > + flags |= EAF_NOESCAPE;
> >
> > likewise.
> >
> > > + if (!fnspec.arg_readonly_p (arg))
> > > + flags |= EAF_NOCLOBBER;
> >
> >
> > likewise.
>
> Oops, sorry for that. I got carried away trying to make sense of
> fortran specifiers. Thanks for catching this. I wonder how that chould
> have passed testing.
> >
> > > }
> > > + return flags;
> > > }
> > >
> > > /* Detects return flags for the call STMT. */
> > > @@ -1546,24 +1542,16 @@ gimple_call_return_flags (const gcall *stmt)
> > > return ERF_NOALIAS;
> > >
> > > attr = gimple_call_fnspec (stmt);
> > > - if (!attr || TREE_STRING_LENGTH (attr) < 1)
> > > + if (!attr)
> > > return 0;
> > > + attr_fnspec fnspec (attr);
> > >
> > > - switch (TREE_STRING_POINTER (attr)[0])
> > > - {
> > > - case '1':
> > > - case '2':
> > > - case '3':
> > > - case '4':
> > > - return ERF_RETURNS_ARG | (TREE_STRING_POINTER (attr)[0] - '1');
> > > -
> > > - case 'm':
> > > - return ERF_NOALIAS;
> > > + if (fnspec.returns_arg () >= 0)
> > > + return ERF_RETURNS_ARG | fnspec.returns_arg ();
> >
> > hmm, maybe
> >
> > if (fnspec.returns_arg_p (&arg))
> > return ERF_RETURNS_ARG | arg;
>
> I added arg variable, but I think returning -1 for unknown arg is kind
> of more consistent with what we do elsewhere (plus referneces are not
> cool)
> > > + /* True if memory reached is only written into (but not read). */
> > > + bool
> > > + arg_writeonly_p (unsigned int i)
> >
> > This is actually arg_readwrite_p (), there's no flag for write-only.
> > wW are merely for noescape & only direct read/write.
>
> I dropped this for now, but for tree-ssa-alias we will need to have way
> to specify that parameter is only written into.
> >
> > Currently all specified args imply noescape btw.
>
> Yep, but I think we may want to change this, so I think it is safer to
> list those that do.
>
> This is updated patch I am re-testing. Does it look OK?
>
> Next I would like to proceed by blowing up all specifiers to double of
> size (without functional changes) and then add the extra letters.
>
> I was wondering if we want to use ' ' instead of '.' for the second
> char. It may make it easier to read ". . . R " than "......R." but it
> also may be bit misleading in a way that the there must be precisely one
> space.
>
> Honza
>
> gcc/ChangeLog:
>
> 2020-10-01 Jan Hubicka <hubi...@ucw.cz>
>
> * attr-fnspec.h: New file.
> * calls.c (decl_return_flags): Implement using attr_fnspec.
> * gimple.c (gimple_call_arg_flags): Likewise
> (gimple_call_return_flags): Likewise
> * tree-into-ssa.c (pass_build_ssa::execute): Likewise.
> * tree-ssa-alias.c (attr_fnspec::verify): New
>
> diff --git a/gcc/attr-fnspec.h b/gcc/attr-fnspec.h
> new file mode 100644
> index 00000000000..4ad4b8758e0
> --- /dev/null
> +++ b/gcc/attr-fnspec.h
> @@ -0,0 +1,141 @@
> +/* Handling of fnspec attribute specifiers
> + Copyright (C) 2008-2020 Free Software Foundation, Inc.
> + Contributed by Richard Guenther <rguent...@suse.de>
> +
> + This file is part of GCC.
> +
> + GCC is free software; you can redistribute it and/or modify
> + under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + GCC is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with GCC; see the file COPYING3. If not see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* Parse string of attribute "fn spec". This is an internal attribute
> + describing side effects of a function as follows:
> +
> + character 0 specifies properties of return values as follows:
> + '1'...'4' specifies number of argument function returns (as in memset)
> + 'm' specifies that returned value is noalias (as in malloc)
> + '.' specifies that nothing is known.
> +
> + character 1+i specifies properties of argument number i as follows:
> + 'x' or 'X' specifies that parameter is unused.
> + 'r' or 'R' specifies that parameter is only read and memory pointed to
> is
> + never dereferenced.
> + 'w' or 'W' specifies that parameter is only written to.
> + '.' specifies that nothing is known.
> + The uppercase letter in addition specifies that parameter
> + is non-escaping. */
> +
> +#ifndef ATTR_FNSPEC_H
> +#define ATTR_FNSPEC_H
> +
> +class attr_fnspec
> +{
> +private:
> + /* fn spec attribute string. */
> + const char *str;
> + /* length of the fn spec string. */
> + const unsigned len;
> + /* Number of characters specifying return value. */
> + const unsigned int return_desc_size = 1;
> + /* Number of characters specifying size. */
> + const unsigned int arg_desc_size = 1;
> +
> + /* Return start of specifier of arg i. */
> + unsigned int arg_idx (int i)
> + {
> + return return_desc_size + arg_desc_size * i;
> + }
> +
> +public:
> + attr_fnspec (const char *str, unsigned len)
> + : str (str), len (len)
> + {
> + if (flag_checking)
> + verify ();
> + }
> + attr_fnspec (const_tree identifier)
> + : str (TREE_STRING_POINTER (identifier)),
> + len (TREE_STRING_LENGTH (identifier))
> + {
> + if (flag_checking)
> + verify ();
> + }
> +
> + /* Return true if arg I is specified. */
> + bool
> + arg_specified_p (unsigned int i)
> + {
> + return len >= arg_idx (i + 1);
> + }
> +
> + /* True if the argument is not dereferenced recursively, thus only
> + directly reachable memory is read or written. */
> + bool
> + arg_direct_p (unsigned int i)
maybe better arg_direct_only_p, you differ with
arg_readonly_p from EAF_NOCLOBBER as well. It's all just
names of course.
> + {
> + unsigned int idx = arg_idx (i);
> + gcc_checking_assert (arg_specified_p (i));
> + return str[idx] == 'R' || str[idx] == 'W';
> + }
> +
> + /* True if argument is used. */
> + bool
> + arg_used_p (unsigned int i)
> + {
> + unsigned int idx = arg_idx (i);
> + gcc_checking_assert (arg_specified_p (i));
> + return str[idx] != 'x' && str[idx] != 'X';
> + }
Hmm, I guess !arg_used_p also implies arg_noescape_p (not "correctly"
handled by gimple_call_arg_flag currently either). So the question
is what "used"/"unused" means - does it mean the argument is
'inspected'? That is, can we have an argument escape but not
read or written to?
Guess this can be sorted out in a followup, your patch doesn't
change current beahvior.
> + /* True if memory reached by the argument is readonly (not clobbered). */
> + bool
> + arg_readonly_p (unsigned int i)
> + {
> + unsigned int idx = arg_idx (i);
> + gcc_checking_assert (arg_specified_p (i));
> + return str[idx] == 'r' || str[idx] == 'R';
> + }
> +
> + /* True if the argument does not escape. */
> + bool
> + arg_noescape_p (unsigned int i)
> + {
> + unsigned int idx = arg_idx (i);
> + gcc_checking_assert (arg_specified_p (i));
> + return str[idx] == 'w' || str[idx] == 'W'
> + || str[idx] == 'R' || str[idx] == 'r';
> + }
> +
> + /* If not equal to -1 then it specifies number of argument returned by
> + the function. */
> + int
> + returns_arg ()
> + {
> + if (str[0] >= '1' && str[0] <= '4')
> + return str[0]-'1';
> + return -1;
> + }
> +
> + /* Nonzero if the return value does not alias with anything. Functions
> + with the malloc attribute have this set on their return value. */
> + int
bool?
OK with this change at least.
Richard.
> + returns_noalias_p ()
> + {
> + return str[0] == 'm';
> + }
> +
> + /* Check validity of the string. */
> + void verify ();
> +};
> +
> +#endif /* ATTR_FNSPEC_H */
> diff --git a/gcc/calls.c b/gcc/calls.c
> index ed4363811c8..40f4863a89b 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -58,6 +58,7 @@ along with GCC; see the file COPYING3. If not see
> #include "attribs.h"
> #include "builtins.h"
> #include "gimple-fold.h"
> +#include "attr-fnspec.h"
>
> #include "tree-pretty-print.h"
>
> @@ -642,25 +643,15 @@ decl_return_flags (tree fndecl)
> if (!attr)
> return 0;
>
> - attr = TREE_VALUE (TREE_VALUE (attr));
> - if (!attr || TREE_STRING_LENGTH (attr) < 1)
> - return 0;
> -
> - switch (TREE_STRING_POINTER (attr)[0])
> - {
> - case '1':
> - case '2':
> - case '3':
> - case '4':
> - return ERF_RETURNS_ARG | (TREE_STRING_POINTER (attr)[0] - '1');
> + attr_fnspec fnspec (TREE_VALUE (TREE_VALUE (attr)));
>
> - case 'm':
> - return ERF_NOALIAS;
> + int arg = fnspec.returns_arg ();
> + if (arg >= 0)
> + return ERF_RETURNS_ARG | arg;
>
> - case '.':
> - default:
> - return 0;
> - }
> + if (fnspec.returns_noalias_p ())
> + return ERF_NOALIAS;
> + return 0;
> }
>
> /* Return nonzero when FNDECL represents a call to setjmp. */
> diff --git a/gcc/gimple.c b/gcc/gimple.c
> index fd4e0fac0d4..8fb82570374 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see
> #include "attribs.h"
> #include "asan.h"
> #include "langhooks.h"
> +#include "attr-fnspec.h"
>
>
> /* All the tuples have their operand vector (if present) at the very bottom
> @@ -1508,31 +1509,26 @@ gimple_call_arg_flags (const gcall *stmt, unsigned
> arg)
> {
> const_tree attr = gimple_call_fnspec (stmt);
>
> - if (!attr || 1 + arg >= (unsigned) TREE_STRING_LENGTH (attr))
> + if (!attr)
> return 0;
>
> - switch (TREE_STRING_POINTER (attr)[1 + arg])
> - {
> - case 'x':
> - case 'X':
> - return EAF_UNUSED;
> -
> - case 'R':
> - return EAF_DIRECT | EAF_NOCLOBBER | EAF_NOESCAPE;
> -
> - case 'r':
> - return EAF_NOCLOBBER | EAF_NOESCAPE;
> -
> - case 'W':
> - return EAF_DIRECT | EAF_NOESCAPE;
> -
> - case 'w':
> - return EAF_NOESCAPE;
> + int flags = 0;
> + attr_fnspec fnspec (attr);
>
> - case '.':
> - default:
> - return 0;
> + if (!fnspec.arg_specified_p (arg))
> + ;
> + else if (!fnspec.arg_used_p (arg))
> + flags = EAF_UNUSED;
> + else
> + {
> + if (fnspec.arg_direct_p (arg))
> + flags |= EAF_DIRECT;
> + if (fnspec.arg_noescape_p (arg))
> + flags |= EAF_NOESCAPE;
> + if (fnspec.arg_readonly_p (arg))
> + flags |= EAF_NOCLOBBER;
> }
> + return flags;
> }
>
> /* Detects return flags for the call STMT. */
> @@ -1546,24 +1542,16 @@ gimple_call_return_flags (const gcall *stmt)
> return ERF_NOALIAS;
>
> attr = gimple_call_fnspec (stmt);
> - if (!attr || TREE_STRING_LENGTH (attr) < 1)
> + if (!attr)
> return 0;
> + attr_fnspec fnspec (attr);
>
> - switch (TREE_STRING_POINTER (attr)[0])
> - {
> - case '1':
> - case '2':
> - case '3':
> - case '4':
> - return ERF_RETURNS_ARG | (TREE_STRING_POINTER (attr)[0] - '1');
> -
> - case 'm':
> - return ERF_NOALIAS;
> + if (fnspec.returns_arg () >= 0)
> + return ERF_RETURNS_ARG | fnspec.returns_arg ();
>
> - case '.':
> - default:
> - return 0;
> - }
> + if (fnspec.returns_noalias_p ())
> + return ERF_NOALIAS;
> + return 0;
> }
>
>
> diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
> index 0d016134774..1493b323956 100644
> --- a/gcc/tree-into-ssa.c
> +++ b/gcc/tree-into-ssa.c
> @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3. If not see
> #include "stringpool.h"
> #include "attribs.h"
> #include "asan.h"
> +#include "attr-fnspec.h"
>
> #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>
> @@ -2492,19 +2493,19 @@ pass_build_ssa::execute (function *fun)
> }
>
> /* Initialize SSA_NAME_POINTS_TO_READONLY_MEMORY. */
> - tree fnspec = lookup_attribute ("fn spec",
> - TYPE_ATTRIBUTES (TREE_TYPE (fun->decl)));
> - if (fnspec)
> + tree fnspec_tree
> + = lookup_attribute ("fn spec",
> + TYPE_ATTRIBUTES (TREE_TYPE (fun->decl)));
> + if (fnspec_tree)
> {
> - fnspec = TREE_VALUE (TREE_VALUE (fnspec));
> - unsigned i = 1;
> + attr_fnspec fnspec (TREE_VALUE (TREE_VALUE (fnspec_tree)));
> + unsigned i = 0;
> for (tree arg = DECL_ARGUMENTS (cfun->decl);
> arg; arg = DECL_CHAIN (arg), ++i)
> {
> - if (i >= (unsigned) TREE_STRING_LENGTH (fnspec))
> - break;
> - if (TREE_STRING_POINTER (fnspec)[i] == 'R'
> - || TREE_STRING_POINTER (fnspec)[i] == 'r')
> + if (!fnspec.arg_specified_p (i))
> + break;
> + if (fnspec.arg_readonly_p (i))
> {
> tree name = ssa_default_def (fun, arg);
> if (name)
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index fe390d4ffbe..2b2d53ebb88 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see
> #include "varasm.h"
> #include "ipa-modref-tree.h"
> #include "ipa-modref.h"
> +#include "attr-fnspec.h"
>
> /* Broad overview of how alias analysis on gimple works:
>
> @@ -3984,3 +3985,42 @@ walk_aliased_vdefs (ao_ref *ref, tree vdef,
> return ret;
> }
>
> +/* Verify validity of the fnspec string.
> + See attr-fnspec.h for details. */
> +
> +void
> +attr_fnspec::verify ()
> +{
> + /* FIXME: Fortran trans-decl.c contains multiple wrong fnspec strings.
> + re-enable verification after these are fixed. */
> + return;
> + bool err = false;
> +
> + /* Check return value specifier. */
> + if (len < return_desc_size)
> + err = true;
> + else if (str[0] < '1' || str[0] > '4')
> + && str[0] != '.' && str[0] != 'm')
> + err = true;
> +
> + /* Now check all parameters. */
> + for (unsigned int i = 0; arg_specified_p (i); i++)
> + {
> + unsigned int idx = arg_idx (i);
> + switch (str[idx])
> + {
> + case 'x':
> + case 'X':
> + case 'r':
> + case 'R':
> + case 'w':
> + case 'W':
> + case '.':
> + break;
> + default:
> + err = true;
> + }
> + }
> + if (err)
> + internal_error ("invalid fn spec attribute %s", str);
> +}
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
