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 <rguent...@suse.de> SUSE / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer