On Tue, 24 Jun 2014, Jan Hubicka wrote:
> > > 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 ...
>
> Yeah, in my testing it seems that C frontend do care in the testcase above -
> we
> used to accept code that does test nonzeroness of address as a constant,
> while we
> don't.
> Clang (IMO correctly) reject it. If we are OK with changing the bhaviour, I
> will
> just commit the patch and remove the testcase above (or turn it into error
> check)
Well, I'd say we ask Joseph.
> >
> > Personally I find "nonzero" ambiguous as it doesn't clearly state
> > it is about the symbols address rather than its value.
>
> I can change it to nonzero_address. It did not appear one might think about
> value stored in the symbol.
Maybe binds_nonzero? It's about how the dynamic loader / link editor
handles the symbol after all.
Please wait for clarification from Joseph.
Thanks,
Richard.
> Honza
> >
> > 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
>
>
--
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
