This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Making fold-const sane WRT symbol visibilities


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]