On Thu, Dec 4, 2014 at 12:28 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> Hi,
> this patch fixes fold-const folding if EQ/NE_EXPR of ADDR_EXPRS WRT aliases
> and
> weaks. Similarly as my earlier nonzero_address_p patch, it moves the logic
> whether two symbls can resolve to same location to symtab.
>
> The existing code did not make much sense to me, so I tried to follow what
> alias oracle does - i.e. if we know both bases are decls, but offsets are
> different, we know the addresses are different.
> If we have offsets same (or we are unsure) we can see if the bases are known
> to be same or different.
>
> One important case that the patch fixes is folding &alias == &alias_target as
> false. This avoids late optimization from removing many of the speculatively
> inlined functions.A
>
> For virtual function I play even bit unsafe and make alias == &alias_target
> even if alias_target is interposable but can not change semantically.
> This helps to turn speculative inlining into nonspeculative late in
> compilation
> and should be safe, becuase virtual functions are not compared for addresses
> in
> other contexts.
>
> Bootstrapped/regtested x86_64-linux, seems sane?
>
> Honza
>
> * c-family/c-common.c: Refuse weak on visibility change.
> * cgraph.h: (sybtam_node::equal_address_to): Declare.
> * fold-const.c (fold_comparison): Use equal_address_to.
> (fold_binary_loc): Likewise.
> * symtab.c (symtab_node::equal_address_to): New predicate.
>
> * gcc.dg/addr_equal-1.c: New testcase.
> Index: c-family/c-common.c
> ===================================================================
> --- c-family/c-common.c (revision 218286)
> +++ c-family/c-common.c (working copy)
> @@ -7781,7 +7781,12 @@
> }
> else if (TREE_CODE (*node) == FUNCTION_DECL
> || TREE_CODE (*node) == VAR_DECL)
> - declare_weak (*node);
> + {
> + struct symtab_node *n = symtab_node::get (*node);
> + if (n && n->refuse_visibility_changes)
> + error ("%+D declared weak after being used", *node);
> + declare_weak (*node);
> + }
> else
> warning (OPT_Wattributes, "%qE attribute ignored", name);
>
> Index: cgraph.h
> ===================================================================
> --- cgraph.h (revision 218286)
> +++ cgraph.h (working copy)
> @@ -332,6 +332,11 @@
> /* Return true if symbol is known to be nonzero. */
> bool nonzero_address ();
>
> + /* Return 0 if symbol is known to have different address than S2,
> + Return 1 if symbol is known to have same address as S2,
> + return 2 otherwise. */
> + int equal_address_to (symtab_node *s2);
> +
> /* Return symbol table node associated with DECL, if any,
> and NULL otherwise. */
> static inline symtab_node *get (const_tree decl)
> Index: fold-const.c
> ===================================================================
> --- fold-const.c (revision 218286)
> +++ fold-const.c (working copy)
> @@ -8980,7 +8981,7 @@
> }
> }
> /* For non-equal bases we can simplify if they are addresses
> - of local binding decls or constants. */
> + declarations with different addresses. */
> else if (indirect_base0 && indirect_base1
> /* We know that !operand_equal_p (base0, base1, 0)
> because the if condition was false. But make
> @@ -8988,16 +8989,13 @@
> && base0 != base1
> && TREE_CODE (arg0) == ADDR_EXPR
> && TREE_CODE (arg1) == ADDR_EXPR
> - && (((TREE_CODE (base0) == VAR_DECL
> - || TREE_CODE (base0) == PARM_DECL)
> - && (targetm.binds_local_p (base0)
> - || CONSTANT_CLASS_P (base1)))
> - || CONSTANT_CLASS_P (base0))
> - && (((TREE_CODE (base1) == VAR_DECL
> - || TREE_CODE (base1) == PARM_DECL)
> - && (targetm.binds_local_p (base1)
> - || CONSTANT_CLASS_P (base0)))
> - || CONSTANT_CLASS_P (base1)))
> + && DECL_P (base0)
> + && DECL_P (base1)
> + /* Watch for aliases. */
> + && (!decl_in_symtab_p (base0)
> + || !decl_in_symtab_p (base1)
> + || !symtab_node::get_create (base0)->equal_address_to
> + (symtab_node::get_create (base1))))
> {
> if (code == EQ_EXPR)
> return omit_two_operands_loc (loc, type, boolean_false_node,
> @@ -12248,39 +12246,6 @@
> && code == NE_EXPR)
> return non_lvalue_loc (loc, fold_convert_loc (loc, type, arg0));
>
> - /* 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)))
> - {
> - /* We know that we're looking at the address of two
> - non-weak, unaliased, static _DECL nodes.
> -
> - It is both wasteful and incorrect to call operand_equal_p
> - to compare the two ADDR_EXPR nodes. It is wasteful in that
> - all we need to do is test pointer equality for the arguments
> - to the two ADDR_EXPR nodes. It is incorrect to use
> - operand_equal_p as that function is NOT equivalent to a
> - C equality test. It can in fact return false for two
> - objects which would test as equal using the C equality
> - operator. */
> - bool equal = TREE_OPERAND (arg0, 0) == TREE_OPERAND (arg1, 0);
> - return constant_boolean_node (equal
> - ? code == EQ_EXPR : code != EQ_EXPR,
> - type);
> - }
> -
> /* Similarly for a NEGATE_EXPR. */
> if (TREE_CODE (arg0) == NEGATE_EXPR
> && TREE_CODE (arg1) == INTEGER_CST
> @@ -12301,6 +12266,104 @@
> arg1),
> TREE_OPERAND (arg0, 1)));
>
> + /* Compare two addresses. */
> + if (TREE_CODE (arg0) == ADDR_EXPR
> + && TREE_CODE (arg1) == ADDR_EXPR)
> + {
> + HOST_WIDE_INT size0, hwi_offset0, max_size0;
> + HOST_WIDE_INT size1, hwi_offset1, max_size1;
> + tree base0 = get_ref_base_and_extent (TREE_OPERAND (arg0, 0),
> &hwi_offset0, &size0, &max_size0);
> + tree base1 = get_ref_base_and_extent (TREE_OPERAND (arg1, 0),
> &hwi_offset1, &size1, &max_size1);
I think you want get_addr_base_and_unit_offset here. But I really wonder
what cases this code catches that the code in fold_comparison you touched
above does not? (apart from previously being bogus in different kind of
ways)
So I'd rather remove the code in fold_binary.
Thanks,
Richard.
> + offset_int offset0 = hwi_offset0;
> + offset_int offset1 = hwi_offset1;
> +
> + /* Add constant offset of MEM_REF to OFFSET, if possible. */
> + if (base0 && TREE_CODE (base0) == MEM_REF
> + && TREE_CODE (TREE_OPERAND (base0, 1)) == INTEGER_CST)
> + {
> + offset0 += wi::lshift (mem_ref_offset (base0),
> LOG2_BITS_PER_UNIT);
> + base0 = TREE_OPERAND (base0, 0);
> + }
> + if (base1 && TREE_CODE (base1) == MEM_REF
> + && TREE_CODE (TREE_OPERAND (base1, 1)) == INTEGER_CST)
> + {
> + offset1 += wi::lshift (mem_ref_offset (base1),
> LOG2_BITS_PER_UNIT);
> + base1 = TREE_OPERAND (base1, 0);
> + }
> + /* If both offsets of MEM_REF are the same, just ignore them. */
> + if (base0 && base1
> + && TREE_CODE (base0) == MEM_REF
> + && TREE_CODE (base1) == MEM_REF
> + && operand_equal_p (TREE_OPERAND (base0, 1), TREE_OPERAND
> (base1, 1), 0))
> + {
> + base0 = TREE_OPERAND (base0, 0);
> + base1 = TREE_OPERAND (base1, 1);
> + }
> + /* If we see MEM_REF with variable offset, just modify MAX_SIZE to
> declare that
> + sizes are unknown. We can still prove that bases points to
> different memory
> + locations. */
> + if (base0 && TREE_CODE (base0) == MEM_REF)
> + {
> + base0 = TREE_OPERAND (base0, 0);
> + max_size0 = -1;
> + }
> + if (base1 && TREE_CODE (base1) == MEM_REF)
> + {
> + base1 = TREE_OPERAND (base1, 0);
> + max_size1 = -1;
> + }
> +
> + /* If bases are equal or they are both declarations, we can
> disprove equivalency by
> + proving that offsets are different. */
> + if (base0 && base1 && (base0 == base1 || (DECL_P (base0) && DECL_P
> (base1))))
> + {
> + if ((wi::ltu_p (offset0 + max_size0 - size0, offset1)
> + || (wi::ltu_p (offset1 + max_size1 - size1, offset0)))
> + && max_size0 != -1 && max_size1 != -1
> + && size0 != -1 && size1 != -1)
> + return constant_boolean_node (code != EQ_EXPR, type);
> + }
> + /* If bases are equal, then addresses are equal if offsets are.
> + We can work hader here for non-constant offsets. */
> + if (base0 && base0 == base1)
> + {
> + if (base0 == base1
> + && size0 != -1 && size1 != -1
> + && (max_size0 == size0) && (max_size1 == size1))
> + return constant_boolean_node (code == EQ_EXPR, type);
> + }
> +
> + if (base0 && base1 && DECL_P (base0) && DECL_P (base1))
> + {
> + bool in_symtab0 = decl_in_symtab_p (base0);
> + bool in_symtab1 = decl_in_symtab_p (base1);
> +
> + /* Symtab and non-symtab declarations never overlap. */
> + if (in_symtab0 != in_symtab1)
> + return constant_boolean_node (code != EQ_EXPR, type);
> + /* Non-symtab nodes never have aliases: different declaration
> means
> + different memory object. */
> + if (!in_symtab0)
> + {
> + if (base0 != base1 && TREE_CODE (base0) != TREE_CODE
> (base1))
> + return constant_boolean_node (code != EQ_EXPR, type);
> + }
> + else
> + {
> + struct symtab_node *symbol0 = symtab_node::get_create
> (base0);
> + struct symtab_node *symbol1 = symtab_node::get_create
> (base1);
> + int cmp = symbol0->equal_address_to (symbol1);
> +
> + if (cmp == 0)
> + return constant_boolean_node (code != EQ_EXPR, type);
> + if (cmp == 1
> + && size0 != -1 && size1 != -1
> + && (max_size0 == size0) && (max_size1 == size1))
> + return constant_boolean_node (code == EQ_EXPR, type);
> + }
> + }
> + }
> +
> /* Transform comparisons of the form X +- Y CMP X to Y CMP 0. */
> if ((TREE_CODE (arg0) == PLUS_EXPR
> || TREE_CODE (arg0) == POINTER_PLUS_EXPR
> Index: symtab.c
> ===================================================================
> --- symtab.c (revision 218286)
> +++ symtab.c (working copy)
> @@ -1860,3 +1860,90 @@
> return true;
> return false;
> }
> +
> +/* Return 0 if symbol is known to have different address than S2,
> + Return 1 if symbol is known to have same address as S2,
> + return 2 otherwise. */
> +int
> +symtab_node::equal_address_to (symtab_node *s2)
> +{
> + enum availability avail1, avail2;
> +
> + /* A Shortcut: equivalent symbols are always equivalent. */
> + if (this == s2)
> + return 1;
> +
> + /* For non-interposable aliases, lookup and compare their actual
> definitions.
> + Also check if the symbol needs to bind to given definition. */
> + symtab_node *rs1 = ultimate_alias_target (&avail1);
> + symtab_node *rs2 = s2->ultimate_alias_target (&avail2);
> + bool binds_local1 = rs1->analyzed && decl_binds_to_current_def_p
> (this->decl);
> + bool binds_local2 = rs2->analyzed && decl_binds_to_current_def_p
> (s2->decl);
> + bool really_binds_local1 = binds_local1;
> + bool really_binds_local2 = binds_local2;
> +
> + /* Addresses of vtables and virtual functions can not be used by user
> + code and are used only within speculation. In this case we may make
> + symbol equivalent to its alias even if interposition may break this
> + rule. Doing so will allow us to turn speculative inlining into
> + non-speculative more agressively. */
> + if (DECL_VIRTUAL_P (this->decl) && avail1 >= AVAIL_AVAILABLE)
> + binds_local1 = true;
> + if (DECL_VIRTUAL_P (s2->decl) && avail2 >= AVAIL_AVAILABLE)
> + binds_local2 = true;
> +
> + /* If both definitions are available we know that even if they are bound
> + to other unit they must be defined same way and therefore we can use
> + equivalence test. */
> + if (rs1 != rs2 && avail1 >= AVAIL_AVAILABLE && avail2 >= AVAIL_AVAILABLE)
> + binds_local1 = binds_local2 = true;
> +
> + if ((binds_local1 ? rs1 : this)
> + == (binds_local2 ? rs2 : s2))
> + {
> + /* We made use of the fact that alias is not weak. */
> + if (binds_local1 && rs1 != this)
> + refuse_visibility_changes = true;
> + if (binds_local2 && rs2 != s2)
> + s2->refuse_visibility_changes = true;
> + return 1;
> + }
> +
> + /* If both symbols may resolve to NULL, we can not really prove them
> different. */
> + if (!nonzero_address () && !s2->nonzero_address ())
> + return 2;
> +
> + /* Except for NULL, functions and variables never overlap. */
> + if (TREE_CODE (decl) != TREE_CODE (s2->decl))
> + return 0;
> +
> + /* If one of the symbols is unresolved alias, punt. */
> + if (rs1->alias || rs2->alias)
> + return 2;
> +
> + /* If we have a non-interposale definition of at least one of the symbols
> + and the other symbol is different, we know other unit can not interpose
> + it to the first symbol; all aliases of the definition needs to be
> + present in the current unit. */
> + if (((really_binds_local1 || really_binds_local2)
> + /* If we have both definitions and they are different, we know they
> + will be different even in units they binds to. */
> + || (binds_local1 && binds_local2))
> + && rs1 != rs2)
> + {
> + /* We make use of the fact that one symbol is not alias of the other
> + and that the definition is non-interposable. */
> + refuse_visibility_changes = true;
> + s2->refuse_visibility_changes = true;
> + rs1->refuse_visibility_changes = true;
> + rs2->refuse_visibility_changes = true;
> + return 0;
> + }
> +
> + /* TODO: Alias oracle basically assume that addresses of global variables
> + are different unless they are declared as alias of one to another.
> + We probably should be consistent and use this fact here, too, and update
> + alias oracle to use this predicate. */
> +
> + return 2;
> +}
> Index: testsuite/gcc.dg/addr_equal-1.c
> ===================================================================
> --- testsuite/gcc.dg/addr_equal-1.c (revision 0)
> +++ testsuite/gcc.dg/addr_equal-1.c (working copy)
> @@ -0,0 +1,107 @@
> +/* { dg-do run } */
> +/* { dg-require-weak "" } */
> +/* { dg-require-alias "" } */
> +/* { dg-options "-O2" } */
> +void abort (void);
> +extern int undef_var0, undef_var1;
> +extern __attribute__ ((weak)) int weak_undef_var0;
> +extern __attribute__ ((weak)) int weak_undef_var1;
> +__attribute__ ((weak)) int weak_def_var0;
> +int def_var0=0, def_var1=0;
> +static int alias_var0 __attribute__ ((alias("def_var0")));
> +extern int weak_alias_var0 __attribute__ ((alias("def_var0"))) __attribute__
> ((weak));
> +void undef_fn0(void);
> +void undef_fn1(void);
> +void def_fn0(void)
> +{
> +}
> +void def_fn1(void)
> +{
> +}
> +__attribute__ ((weak))
> +void weak_def_fn0(void)
> +{
> +}
> +__attribute__ ((weak))
> +void weak_def_fn1(void)
> +{
> +}
> +__attribute__ ((weak)) void weak_undef_fn0(void);
> +
> +inline
> +void inline_fn0(void)
> +{
> +}
> +inline
> +void inline_fn1(void)
> +{
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + /* Two definitions are always different unless they can be interposed. */
> + if (!__builtin_constant_p (def_fn0 == def_fn1))
> + abort();
> + if (def_fn0 == def_fn1)
> + abort();
> +
> + if (!__builtin_constant_p (&def_var0 == &def_var1))
> + abort();
> + if (&def_var0 == &def_var1)
> + abort();
> +
> + /* Same symbol is the same no matter on interposition. */
> + if (!__builtin_constant_p (undef_fn0 != undef_fn0))
> + abort ();
> + if (undef_fn0 != undef_fn0)
> + abort ();
> +
> + /* Do not get confused by same offset. */
> + if (!__builtin_constant_p (&undef_var0 + argc != &undef_var0 + argc))
> + abort ();
> + if (&undef_var0 + argc != &undef_var0 + argc)
> + abort ();
> +
> + /* Alias and its target is equivalent unless one of them can be
> interposed. */
> + if (!__builtin_constant_p (&def_var0 != &alias_var0))
> + abort ();
> + if (&def_var0 != &alias_var0 )
> + abort ();
> +
> + if (__builtin_constant_p (&def_var0 != &weak_alias_var0))
> + abort ();
> + if (&def_var0 != &weak_alias_var0)
> + abort ();
> +
> + /* Weak definitions may be both NULL. */
> + if (__builtin_constant_p ((void *)weak_undef_fn0 == (void
> *)&weak_undef_var0))
> + abort ();
> + if ((void *)weak_undef_fn0 != (void *)&weak_undef_var0)
> + abort ();
> +
> + /* Different offsets makes it safe to assume addresses are different. */
> + if (!__builtin_constant_p ((char *)weak_undef_fn0 + 4 != (char
> *)&weak_undef_var1 + 8))
> + abort ();
> + if ((char *)weak_undef_fn0 + 4 == (char *)&weak_undef_var1 + 8)
> + abort ();
> +
> + /* Variables and functions do not share same memory locations otherwise.
> */
> + if (!__builtin_constant_p ((void *)undef_fn0 == (void *)&undef_var0))
> + abort ();
> + if ((void *)undef_fn0 == (void *)&undef_var0)
> + abort ();
> +
> + /* This works for cases where one object is just weakly defined, too. */
> + if (!__builtin_constant_p ((void *)weak_undef_fn0 == (void
> *)&weak_def_var0))
> + abort ();
> + if ((void *)weak_undef_fn0 == (void *)&weak_def_var0)
> + abort ();
> +
> + /* Inline functions are known to be different. */
> + if (!__builtin_constant_p (inline_fn0 != inline_fn1))
> + abort ();
> + if (inline_fn0 == inline_fn1)
> + abort ();
> + return 0;
> +}
