On Tue, 24 Jun 2014, Jan Hubicka wrote:
> Hello,
> fold-const contains quite few confused statements that deal with WEAK
> visibility and aliases:
>
> static int
> simple_operand_p (const_tree exp)
> {
> /* Strip any conversions that don't change the machine mode. */
> STRIP_NOPS (exp);
>
> return (CONSTANT_CLASS_P (exp)
> || TREE_CODE (exp) == SSA_NAME
> || (DECL_P (exp)
> && ! TREE_ADDRESSABLE (exp)
> && ! TREE_THIS_VOLATILE (exp)
> && ! DECL_NONLOCAL (exp)
> /* Don't regard global variables as simple. They may be
> allocated in ways unknown to the compiler (shared memory,
> #pragma weak, etc). */
> && ! TREE_PUBLIC (exp)
> && ! DECL_EXTERNAL (exp)
> /* Weakrefs are not safe to be read, since they can be NULL.
> They are !TREE_PUBLIC && !DECL_EXTERNAL but still
> have DECL_WEAK flag set. */
> && (! VAR_OR_FUNCTION_DECL_P (exp) || ! DECL_WEAK (exp))
> /* Loading a static variable is unduly expensive, but global
> registers aren't expensive. */
> && (! TREE_STATIC (exp) || DECL_REGISTER (exp))));
> }
>
> Here I think WEAK is useless, since we already check PUBLIC.
> /* If this is an equality comparison of the address of two non-weak,
> unaliased symbols neither of which are extern (since we do not
> have access to attributes for externs), then we know the result. */
> if (TREE_CODE (arg0) == ADDR_EXPR
> && VAR_OR_FUNCTION_DECL_P (TREE_OPERAND (arg0, 0))
> && ! DECL_WEAK (TREE_OPERAND (arg0, 0))
> && ! lookup_attribute ("alias",
> DECL_ATTRIBUTES (TREE_OPERAND (arg0, 0)))
> && ! DECL_EXTERNAL (TREE_OPERAND (arg0, 0))
> && TREE_CODE (arg1) == ADDR_EXPR
> && VAR_OR_FUNCTION_DECL_P (TREE_OPERAND (arg1, 0))
> && ! DECL_WEAK (TREE_OPERAND (arg1, 0))
> && ! lookup_attribute ("alias",
> DECL_ATTRIBUTES (TREE_OPERAND (arg1, 0)))
> && ! DECL_EXTERNAL (TREE_OPERAND (arg1, 0)))
>
> Here we no longer consstently use alias attribute to do aliases - it should
> ask symtab. Moreover it handle just fraction of cases where we can prove
> nonequality.
> and some others.
>
> This patch attempts to deal with nonzero that is one of basic predicates. I
> added
> symtab_node::nonzero that I hope is implemented safely and I removed the
> fold-const
> code (I will cleanup a bit its implementation - at the very end I added the
> flag_ checks that makes it osmewhat ugly).
> The problem is that I do not think I can safely fold before symtab is
> constructed
> as shown by the testcase:
>
> extern int a;
> t()
> {
> return &a!=0;
> }
> extern int a __attribute__ ((weak));
>
> Here we incorrectly fold into true before we see the weak predicate.
>
> The problem is that the patch fails testcases that assume we do such folding
> at parsing
> time.
>
> ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-1.c (test for excess errors)
> ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-2.c (test for excess errors)
> ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-3.c (test for excess errors)
> ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-4.c (test for excess errors)
>
> Here we accept the source as compile time constant while I think it is not:
> int sc = (&sc > 0);
>
> Does it seem resonable to turn those testcases into one testing for error and
> live with delaying the folding oppurtunities to early opts? They are now
> caught
> by ccp1 pass usually.
IMHO all symbol visibility related foldings are very premature if done
in the frontends (well, most of fold-const.c is ...). Of course
everything depends on whether there exists a frontend that requires
these foldings for correctness ...
Personally I find "nonzero" ambiguous as it doesn't clearly state
it is about the symbols address rather than its value.
Richard.
> Honza
>
> * cgraph.h (symtab_node): Add method nonzero.
> (decl_in_symtab_p): Break out from ...
> (symtab_get_node): ... here.
> * symtab.c (symtab_node::nonzero): New method.
> * fold-const.c: Include cgraph.h
> (tree_single_nonzero_warnv_p): Use symtab for symbols.
>
> * testsuite/g++.dg/tree-ssa/nonzero-2.C: New testcase.
> * testsuite/g++.dg/tree-ssa/nonzero-1.C: New testcase.
> * testsuite/gcc.dg/tree-ssa/nonzero-1.c: New testcase.
> Index: cgraph.h
> ===================================================================
> --- cgraph.h (revision 211915)
> +++ cgraph.h (working copy)
> @@ -214,6 +214,9 @@ public:
>
> void set_init_priority (priority_type priority);
> priority_type get_init_priority ();
> +
> + /* Return true if symbol is known to be nonzero. */
> + bool nonzero ();
> };
>
> enum availability
> @@ -1068,6 +1077,17 @@ void varpool_remove_initializer (varpool
> /* In cgraph.c */
> extern void change_decl_assembler_name (tree, tree);
>
> +/* Return true if DECL should have entry in symbol table if used.
> + Those are functions and static & external veriables*/
> +
> +static bool
> +decl_in_symtab_p (const_tree decl)
> +{
> + return (TREE_CODE (decl) == FUNCTION_DECL
> + || (TREE_CODE (decl) == VAR_DECL
> + && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))));
> +}
> +
> /* Return symbol table node associated with DECL, if any,
> and NULL otherwise. */
>
> @@ -1075,12 +1095,7 @@ static inline symtab_node *
> symtab_get_node (const_tree decl)
> {
> #ifdef ENABLE_CHECKING
> - /* Check that we are called for sane type of object - functions
> - and static or external variables. */
> - gcc_checking_assert (TREE_CODE (decl) == FUNCTION_DECL
> - || (TREE_CODE (decl) == VAR_DECL
> - && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)
> - || in_lto_p)));
> + gcc_checking_assert (decl_in_symtab_p (decl));
> /* Check that the mapping is sane - perhaps this check can go away,
> but at the moment frontends tends to corrupt the mapping by calling
> memcpy/memset on the tree nodes. */
> Index: symtab.c
> ===================================================================
> --- symtab.c (revision 211915)
> +++ symtab.c (working copy)
> @@ -1574,4 +1574,67 @@ symtab_get_symbol_partitioning_class (sy
>
> return SYMBOL_PARTITION;
> }
> +
> +/* Return true when symbol is known to be non-zero. */
> +
> +bool
> +symtab_node::nonzero ()
> +{
> + /* Weakrefs may be NULL when their target is not defined. */
> + if (this->alias && this->weakref)
> + {
> + if (this->analyzed)
> + {
> + symtab_node *target = symtab_alias_ultimate_target (this);
> +
> + if (target->alias && target->weakref)
> + return false;
> + /* We can not recurse to target::nonzero. It is possible that the
> + target is used only via the alias.
> + We may walk references and look for strong use, but we do not know
> + if this strong use will survive to final binary, so be
> + conservative here.
> + ??? Maybe we could do the lookup during late optimization that
> + could be useful to eliminate the NULL pointer checks in LTO
> + programs. */
> + if (target->definition && !DECL_EXTERNAL (target->decl))
> + return true;
> + if (target->resolution != LDPR_UNKNOWN
> + && target->resolution != LDPR_UNDEF
> + && flag_delete_null_pointer_checks)
> + return true;
> + return false;
> + }
> + else
> + return false;
> + }
> +
> + /* With !flag_delete_null_pointer_checks we assume that symbols may
> + bind to NULL. This is on by default on embedded targets only.
> +
> + Otherwise all non-WEAK symbols must be defined and thus non-NULL or
> + linking fails. Important case of WEAK we want to do well are comdats.
> + Those are handled by later check for definition.
> +
> + When parsing, beware the cases when WEAK attribute is added later. */
> + if (!DECL_WEAK (this->decl)
> + && flag_delete_null_pointer_checks
> + && cgraph_state > CGRAPH_STATE_PARSING)
> + return true;
> +
> + /* If target is defined and not extern, we know it will be output and thus
> + it will bind to non-NULL.
> + Play safe for flag_delete_null_pointer_checks where weak definition maye
> + be re-defined by NULL. */
> + if (this->definition && !DECL_EXTERNAL (this->decl)
> + && (flag_delete_null_pointer_checks || !DECL_WEAK (this->decl)))
> + return true;
> +
> + /* As the last resort, check the resolution info. */
> + if (this->resolution != LDPR_UNKNOWN
> + && this->resolution != LDPR_UNDEF
> + && flag_delete_null_pointer_checks)
> + return true;
> + return false;
> +}
> #include "gt-symtab.h"
> Index: fold-const.c
> ===================================================================
> --- fold-const.c (revision 211915)
> +++ fold-const.c (working copy)
> @@ -69,6 +69,7 @@ along with GCC; see the file COPYING3.
> #include "tree-dfa.h"
> #include "hash-table.h" /* Required for ENABLE_FOLD_CHECKING. */
> #include "builtins.h"
> +#include "cgraph.h"
>
> /* Nonzero if we are folding constants inside an initializer; zero
> otherwise. */
> @@ -16032,21 +16033,33 @@ tree_single_nonzero_warnv_p (tree t, boo
> case ADDR_EXPR:
> {
> tree base = TREE_OPERAND (t, 0);
> +
> if (!DECL_P (base))
> base = get_base_address (base);
>
> if (!base)
> return false;
>
> - /* Weak declarations may link to NULL. Other things may also be NULL
> - so protect with -fdelete-null-pointer-checks; but not variables
> - allocated on the stack. */
> + /* For objects in symbol table check if we know they are non-zero.
> + Don't do anything for variables and functions before symtab is built;
> + it is quite possible that they will be declared weak later. */
> + if (DECL_P (base) && decl_in_symtab_p (base))
> + {
> + struct symtab_node *symbol;
> +
> + symbol = symtab_get_node (base);
> + if (symbol)
> + return symbol->nonzero ();
> + else
> + return false;
> + }
> +
> + /* Function local objects are never NULL. */
> if (DECL_P (base)
> - && (flag_delete_null_pointer_checks
> - || (DECL_CONTEXT (base)
> - && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL
> - && auto_var_in_fn_p (base, DECL_CONTEXT (base)))))
> - return !VAR_OR_FUNCTION_DECL_P (base) || !DECL_WEAK (base);
> + && (DECL_CONTEXT (base)
> + && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL
> + && auto_var_in_fn_p (base, DECL_CONTEXT (base))))
> + return true;
>
> /* Constants are never weak. */
> if (CONSTANT_CLASS_P (base))
> Index: testsuite/gcc.dg/tree-ssa/nonzero-1.c
> ===================================================================
> --- testsuite/gcc.dg/tree-ssa/nonzero-1.c (revision 0)
> +++ testsuite/gcc.dg/tree-ssa/nonzero-1.c (revision 0)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +extern int a;
> +t()
> +{
> + return &a!=0;
> +}
> +extern int a __attribute__ ((weak));
> +
> +/* { dg-final { scan-tree-dump-not "return 1" "optimized"} } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: testsuite/g++.dg/tree-ssa/nonzero-1.C
> ===================================================================
> --- testsuite/g++.dg/tree-ssa/nonzero-1.C (revision 0)
> +++ testsuite/g++.dg/tree-ssa/nonzero-1.C (revision 0)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-ccp1" } */
> +inline void t()
> +{
> +}
> +int m()
> +{
> + void *q = (void *)&t;
> + return q != 0;
> +}
> +/* { dg-final { scan-tree-dump "return 1" "ccp1"} } */
> +/* { dg-final { cleanup-tree-dump "ccp1" } } */
> Index: testsuite/g++.dg/tree-ssa/nonzero-2.C
> ===================================================================
> --- testsuite/g++.dg/tree-ssa/nonzero-2.C (revision 0)
> +++ testsuite/g++.dg/tree-ssa/nonzero-2.C (revision 0)
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-ccp1 -fdelete-null-pointer-checks" } */
> +struct t
> +{
> + static inline void tt()
> + {
> + }
> + virtual void q();
> +};
> +int m()
> +{
> + void *q = (void *)&t::tt;
> + return q != 0;
> +}
> +/* { dg-final { scan-tree-dump "return 1" "ccp1"} } */
> +/* { dg-final { cleanup-tree-dump "ccp1" } } */
>
>
--
Richard Biener <[email protected]>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
