| Message ID | 1984635.2VDvgg1PtU@polaris |
|---|---|
| State | New |
| Headers | show |
Hi, Sorry for jumping in late, only now I had chance to read through the whole discussion. I was looking into similar problem some time ago. > Index: lto-streamer-out.c > =================================================================== > --- lto-streamer-out.c (revision 238156) > +++ lto-streamer-out.c (working copy) > @@ -996,7 +996,9 @@ hash_tree (struct streamer_tree_cache_d > else > hstate.add_flag (TREE_NO_WARNING (t)); > hstate.add_flag (TREE_NOTHROW (t)); > - hstate.add_flag (TREE_STATIC (t)); > + /* We want to unify DECL nodes in pointer fields of global types. */ > + if (!(VAR_OR_FUNCTION_DECL_P (t))) > + hstate.add_flag (TREE_STATIC (t)); > hstate.add_flag (TREE_PROTECTED (t)); > hstate.add_flag (TREE_DEPRECATED (t)); > if (code != TREE_BINFO) > @@ -1050,7 +1052,9 @@ hash_tree (struct streamer_tree_cache_d > hstate.add_flag (DECL_ARTIFICIAL (t)); > hstate.add_flag (DECL_USER_ALIGN (t)); > hstate.add_flag (DECL_PRESERVE_P (t)); > - hstate.add_flag (DECL_EXTERNAL (t)); > + /* We want to unify DECL nodes in pointer fields of global types. */ > + if (!(VAR_OR_FUNCTION_DECL_P (t))) > + hstate.add_flag (DECL_EXTERNAL (t)); It is fine to merge decls across static and external flags, but I am not sure this is a safe solution to the problem. In C it is perfectly normal to have one decl more specified or with different attributes. Like: extern int a __attribute__ ((warning("bar")); int a=7 __attribute__ ((warning("foo"))); which must prevent merging otherwise the warnings will go out wrong way. In GCC 6 timeframe we decided to live with duplicated declarations and added support for syntactic aliases. Tree merging should be optional WRT correctness, so I think bug is in the canonical type calculation: /* For array types hash the domain bounds and the string flag. */ if (TREE_CODE (type) == ARRAY_TYPE && TYPE_DOMAIN (type)) { hstate.add_int (TYPE_STRING_FLAG (type)); /* OMP lowering can introduce error_mark_node in place of random local decls in types. */ if (TYPE_MIN_VALUE (TYPE_DOMAIN (type)) != error_mark_node) inchash::add_expr (TYPE_MIN_VALUE (TYPE_DOMAIN (type)), hstate); if (TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node) inchash::add_expr (TYPE_MAX_VALUE (TYPE_DOMAIN (type)), hstate); } which boils down to: if (tclass == tcc_declaration) { /* DECL's have a unique ID */ hstate.add_wide_int (DECL_UID (t)); } (in asd_expr) It is bit ugly, but I think for static/external/public decls we need to use assembler name here (as we can't rely on symtab to be around all the time) which will result in unstability WRT symbol renaming and also give false positives for static symbols. But even for static symbols it is not guaranteed they are not duplicated. It may be possible to use combination of assembler name and translation unit that is available from symtab node, but not all decls will have it defined. Honza
> It is fine to merge decls across static and external flags, but I am not > sure this is a safe solution to the problem. In C it is perfectly normal to > have one decl more specified or with different attributes. Like: > > extern int a __attribute__ ((warning("bar")); > > int a=7 __attribute__ ((warning("foo"))); > > which must prevent merging otherwise the warnings will go out wrong way. In > GCC 6 timeframe we decided to live with duplicated declarations and added > support for syntactic aliases. Tree merging should be optional WRT > correctness, so I think bug is in the canonical type calculation: > > /* For array types hash the domain bounds and the string flag. */ > if (TREE_CODE (type) == ARRAY_TYPE && TYPE_DOMAIN (type)) > { > hstate.add_int (TYPE_STRING_FLAG (type)); > /* OMP lowering can introduce error_mark_node in place of > random local decls in types. */ > if (TYPE_MIN_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MIN_VALUE (TYPE_DOMAIN (type)), hstate); > if (TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MAX_VALUE (TYPE_DOMAIN (type)), hstate); > } > > which boils down to: > > if (tclass == tcc_declaration) > { > /* DECL's have a unique ID */ > hstate.add_wide_int (DECL_UID (t)); > } > > (in asd_expr) > > It is bit ugly, but I think for static/external/public decls we need to use > assembler name here (as we can't rely on symtab to be around all the time) > which will result in unstability WRT symbol renaming and also give false > positives for static symbols. But even for static symbols it is not > guaranteed they are not duplicated. It may be possible to use combination > of assembler name and translation unit that is available from symtab node, > but not all decls will have it defined. So something akin to what I initially proposed? https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00182.html And, as mentioned in my previous message, I'm not sure that we would be able to merge all the global types with the unification scheme, it is too strict.
> > So something akin to what I initially proposed? > https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00182.html I have similar patch to add_expr. I don't think it should have flag_wpa guard: other FEs are keeping umberged decls, too, just not as often as LTO. I am not sure about operand_equal_p. If one decl has warning on it and the other doesn't we can't unify their uses even if assembler name is the same because we may end up messing up the warning. I guess we need to wind up more involved decl comparing here listing properties of decls that needs to match and it may be bit fun determining which ones we want to support. (naturally this is also important for ICF that is also currently wrong WRT warning attributes) I see that gimple_canonical_types_compatible_p winds up using operand_equal_p on expressions which represent the array size and that is why you need walk_simple_constant_arithmetic and operand_equal_p change. This seem conceptually wrong because operand_equal_p is allowed to have false negatives, but not false positives. For TBAA purposes we can tolrate false positives, but no false negatives. I wonder how much code quality we would lose by simply treating only constantly sized arrays and considering all VLAs of the same type to be compatible? There are interesting issues WRT C standard and the canonical types of arrays have just secondary role (i.e. they are hardly used by themselves and only are used to compute canonical types of structures) and there may be lower hanging fruits in TBAA improvement than trying to handle sturctures containng VLAs right. Honza
> I see that gimple_canonical_types_compatible_p winds up using > operand_equal_p on expressions which represent the array size and that is > why you need walk_simple_constant_arithmetic and operand_equal_p change. Not just array size, but also offsets via gimple_compare_field_offset. > I wonder how much code quality we would lose by simply treating only > constantly sized arrays and considering all VLAs of the same type to be > compatible? There are interesting issues WRT C standard and the canonical > types of arrays have just secondary role (i.e. they are hardly used by > themselves and only are used to compute canonical types of structures) and > there may be lower hanging fruits in TBAA improvement than trying to handle > sturctures containng VLAs right. Yes, array types themselves can probably be kludged around, but the main issue in Ada are record types with dynamic offsets, which are first-class citizens.
On Mon, Jul 11, 2016 at 3:28 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> So sth like >> >> Index: gcc/lto-streamer-out.c >> =================================================================== >> --- gcc/lto-streamer-out.c (revision 238039) >> +++ gcc/lto-streamer-out.c (working copy) >> @@ -996,7 +996,7 @@ hash_tree (struct streamer_tree_cache_d >> else >> hstate.add_flag (TREE_NO_WARNING (t)); >> hstate.add_flag (TREE_NOTHROW (t)); >> - hstate.add_flag (TREE_STATIC (t)); >> + //hstate.add_flag (TREE_STATIC (t)); >> hstate.add_flag (TREE_PROTECTED (t)); >> hstate.add_flag (TREE_DEPRECATED (t)); >> if (code != TREE_BINFO) >> @@ -1050,7 +1050,7 @@ hash_tree (struct streamer_tree_cache_d >> hstate.add_flag (DECL_ARTIFICIAL (t)); >> hstate.add_flag (DECL_USER_ALIGN (t)); >> hstate.add_flag (DECL_PRESERVE_P (t)); >> - hstate.add_flag (DECL_EXTERNAL (t)); >> + //hstate.add_flag (DECL_EXTERNAL (t)); >> hstate.add_flag (DECL_GIMPLE_REG_P (t)); >> hstate.commit_flag (); >> hstate.add_int (DECL_ALIGN (t)); >> Index: gcc/lto/lto.c >> =================================================================== >> --- gcc/lto/lto.c (revision 238039) >> +++ gcc/lto/lto.c (working copy) >> @@ -1263,7 +1263,8 @@ compare_tree_sccs_1 (tree t1, tree t2, t >> tree t1_ = (E1), t2_ = (E2); \ >> if (t1_ != t2_ \ >> && (!t1_ || !t2_ \ >> - || !TREE_VISITED (t2_) \ >> + || (!TREE_VISITED (t2_) \ >> + && !same_decls (t1_, t2_)) \ >> >> || (!TREE_ASM_WRITTEN (t2_) \ >> >> && !compare_tree_sccs_1 (t1_, t2_, map)))) \ >> return false; \ >> >> plus the magic new function same_decls which would check if the trees are >> var/function decls with the same assembler name. The function can't use >> the cgraph as that is not yet read in unfortunately - I still think we >> should do that so we can see the prevailing nodes early. > > OK, thanks for the hint, the attached patch where the call to same_decls is > placed differently works on the testcase (modulo the gigi change I posted > earlier today) but not on the original code... It looks like the hash value > computed in lto-streamer-out.c is stricter than the hash value computed for > canonical types, I'm trying to pinpoint the difference. As tree merging really replaces trees it has to error on the side of not merging while canonical type merging has to error on the side of "merging" to make types alias. Btw, for the LTO_SET_PREVAIL can you introduce a LTO_SET_PREVAIL_EXPR and use that for the fields that possibly can be expressions plus simply use walk_tree for those (in case they are EXPR_P)? Please split that part out as well. Richard. > > * lto-streamer-out.c (hash_tree): Do not add the TREE_STATIC and > DECL_EXTERNAL flags for variable or function declarations. > lto/ > * lto.c (same_global_decl_p): New predicate. > (compare_tree_sccs_1): Use it in the comparison of pointer fields. > (walk_simple_constant_arithmetic): New function. > (LTO_SET_PREVAIL): Use it to fix up DECL nodes in simple expressions. > > -- > Eric Botcazou
On Mon, Jul 11, 2016 at 5:14 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > Sorry for jumping in late, only now I had chance to read through the whole discussion. > I was looking into similar problem some time ago. > >> Index: lto-streamer-out.c >> =================================================================== >> --- lto-streamer-out.c (revision 238156) >> +++ lto-streamer-out.c (working copy) >> @@ -996,7 +996,9 @@ hash_tree (struct streamer_tree_cache_d >> else >> hstate.add_flag (TREE_NO_WARNING (t)); >> hstate.add_flag (TREE_NOTHROW (t)); >> - hstate.add_flag (TREE_STATIC (t)); >> + /* We want to unify DECL nodes in pointer fields of global types. */ >> + if (!(VAR_OR_FUNCTION_DECL_P (t))) >> + hstate.add_flag (TREE_STATIC (t)); >> hstate.add_flag (TREE_PROTECTED (t)); >> hstate.add_flag (TREE_DEPRECATED (t)); >> if (code != TREE_BINFO) >> @@ -1050,7 +1052,9 @@ hash_tree (struct streamer_tree_cache_d >> hstate.add_flag (DECL_ARTIFICIAL (t)); >> hstate.add_flag (DECL_USER_ALIGN (t)); >> hstate.add_flag (DECL_PRESERVE_P (t)); >> - hstate.add_flag (DECL_EXTERNAL (t)); >> + /* We want to unify DECL nodes in pointer fields of global types. */ >> + if (!(VAR_OR_FUNCTION_DECL_P (t))) >> + hstate.add_flag (DECL_EXTERNAL (t)); > > It is fine to merge decls across static and external flags, but I am not sure this is > a safe solution to the problem. In C it is perfectly normal to have one decl more specified > or with different attributes. Like: > > extern int a __attribute__ ((warning("bar")); > > int a=7 __attribute__ ((warning("foo"))); > > which must prevent merging otherwise the warnings will go out wrong way. In GCC > 6 timeframe we decided to live with duplicated declarations and added support > for syntactic aliases. Tree merging should be optional WRT correctness, so I think > bug is in the canonical type calculation: > > /* For array types hash the domain bounds and the string flag. */ > if (TREE_CODE (type) == ARRAY_TYPE && TYPE_DOMAIN (type)) > { > hstate.add_int (TYPE_STRING_FLAG (type)); > /* OMP lowering can introduce error_mark_node in place of > random local decls in types. */ > if (TYPE_MIN_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MIN_VALUE (TYPE_DOMAIN (type)), hstate); > if (TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node) > inchash::add_expr (TYPE_MAX_VALUE (TYPE_DOMAIN (type)), hstate); > } > > which boils down to: > > if (tclass == tcc_declaration) > { > /* DECL's have a unique ID */ > hstate.add_wide_int (DECL_UID (t)); > } > > (in asd_expr) Well, I think the patch wouldn't merge those it simply treats them the same for references to outside of a tree SCC. So it's kind of a hack... I think the real fix is to apply the linker resolution to the cgraph early (which means reading the cgraph before reading global decls), keeping a decl-ref to symtab node hash for the purpose of tree merging. > It is bit ugly, but I think for static/external/public decls we need to use > assembler name here (as we can't rely on symtab to be around all the time) > which will result in unstability WRT symbol renaming and also give false > positives for static symbols. But even for static symbols it is not guaranteed > they are not duplicated. It may be possible to use combination of assembler > name and translation unit that is available from symtab node, but not all decls > will have it defined. Yeah, but assembler name is in the cgraph which is not read in yet... (so I wonder how same_global_decl_p in the patch works ...). Oh, it isn't yet in the symbol table... Still I guess it should only compare if DECL_ASSEMBLER_NAME_SET_P. And I think the patch (without the LTO_SET_PREVAIL bits) is sensible. Richard. > > Honza
> As tree merging really replaces trees it has to error on the side of not > merging while canonical type merging has to error on the side of "merging" > to make types alias. OK, then the former won't be sufficient for Ada, there are known cases where producers and clients of a package cannot see the exact same tree for a type, so we definitely need optimistic merging here. > Btw, for the LTO_SET_PREVAIL can you introduce a LTO_SET_PREVAIL_EXPR > and use that for the fields that possibly can be expressions plus simply use > walk_tree for those (in case they are EXPR_P)? Please split that part out > as well. Yes, I can do that.
On Wed, Jul 13, 2016 at 3:16 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> As tree merging really replaces trees it has to error on the side of not >> merging while canonical type merging has to error on the side of "merging" >> to make types alias. > > OK, then the former won't be sufficient for Ada, there are known cases where > producers and clients of a package cannot see the exact same tree for a type, > so we definitely need optimistic merging here. Yeah, as said such optimistic merging would need to happen during canoncial type merging for example by completely ignoring TYPE_DOMAIN or DECL_FIELD_OFFSET ... (a non-constant in one unit may be the same as a constant in another...). But merging more trees is also good - though in this case I tend to favor the approach to have symbols merged first and only the prevailing 'tree' survive. >> Btw, for the LTO_SET_PREVAIL can you introduce a LTO_SET_PREVAIL_EXPR >> and use that for the fields that possibly can be expressions plus simply use >> walk_tree for those (in case they are EXPR_P)? Please split that part out >> as well. > > Yes, I can do that. Thanks. > -- > Eric Botcazou
> Yeah, as said such optimistic merging would need to happen during canoncial > type merging for example by completely ignoring TYPE_DOMAIN or > DECL_FIELD_OFFSET ... (a non-constant in one unit may be the same as a > constant in another...). OK, that's really optimistic, and we can probably get away without going this far, e.g. by completely ignoring non-constant fields only, but I see, thanks.
Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 238156) +++ lto-streamer-out.c (working copy) @@ -996,7 +996,9 @@ hash_tree (struct streamer_tree_cache_d else hstate.add_flag (TREE_NO_WARNING (t)); hstate.add_flag (TREE_NOTHROW (t)); - hstate.add_flag (TREE_STATIC (t)); + /* We want to unify DECL nodes in pointer fields of global types. */ + if (!(VAR_OR_FUNCTION_DECL_P (t))) + hstate.add_flag (TREE_STATIC (t)); hstate.add_flag (TREE_PROTECTED (t)); hstate.add_flag (TREE_DEPRECATED (t)); if (code != TREE_BINFO) @@ -1050,7 +1052,9 @@ hash_tree (struct streamer_tree_cache_d hstate.add_flag (DECL_ARTIFICIAL (t)); hstate.add_flag (DECL_USER_ALIGN (t)); hstate.add_flag (DECL_PRESERVE_P (t)); - hstate.add_flag (DECL_EXTERNAL (t)); + /* We want to unify DECL nodes in pointer fields of global types. */ + if (!(VAR_OR_FUNCTION_DECL_P (t))) + hstate.add_flag (DECL_EXTERNAL (t)); hstate.add_flag (DECL_GIMPLE_REG_P (t)); hstate.commit_flag (); hstate.add_int (DECL_ALIGN (t)); Index: lto/lto.c =================================================================== --- lto/lto.c (revision 238156) +++ lto/lto.c (working copy) @@ -970,6 +970,27 @@ static unsigned long num_sccs_merged; static unsigned long num_scc_compares; static unsigned long num_scc_compare_collisions; +/* Return true if T1 and T2 represent the same global declaration. + + We need to unify declaration nodes in pointer fields of global types + so as to unify the types themselves before the declarations are merged. */ + +static inline bool +same_global_decl_p (tree t1, tree t2) +{ + if (TREE_CODE (t1) != TREE_CODE (t2) || !VAR_OR_FUNCTION_DECL_P (t1)) + return false; + + if (!(TREE_PUBLIC (t1) || DECL_EXTERNAL (t1))) + return false; + + if (!(TREE_PUBLIC (t2) || DECL_EXTERNAL (t2))) + return false; + + return symbol_table::assembler_names_equal_p + (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t1)), + IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t2))); +} /* Compare the two entries T1 and T2 of two SCCs that are possibly equal, recursing through in-SCC tree edges. Returns true if the SCCs entered @@ -1262,6 +1283,7 @@ compare_tree_sccs_1 (tree t1, tree t2, t do { \ tree t1_ = (E1), t2_ = (E2); \ if (t1_ != t2_ \ + && !(t1_ && t2_ && same_global_decl_p (t1_, t2_)) \ && (!t1_ || !t2_ \ || !TREE_VISITED (t2_) \ || (!TREE_ASM_WRITTEN (t2_) \ @@ -2506,15 +2528,55 @@ lto_wpa_write_files (void) timevar_pop (TV_WHOPR_WPA_IO); } +/* Look inside *EXPR into simple arithmetic operations involving constants. + Return the address of the outermost non-arithmetic or non-constant node. + This should be equivalent to tree.c:skip_simple_constant_arithmetic. */ + +static tree * +walk_simple_constant_arithmetic (tree *expr) +{ + while (TREE_CODE (*expr) == NON_LVALUE_EXPR) + expr = &TREE_OPERAND (*expr, 0); + + while (true) + { + if (UNARY_CLASS_P (*expr)) + expr = &TREE_OPERAND (*expr, 0); + else if (BINARY_CLASS_P (*expr)) + { + if (TREE_CONSTANT (TREE_OPERAND (*expr, 1))) + expr = &TREE_OPERAND (*expr, 0); + else if (TREE_CONSTANT (TREE_OPERAND (*expr, 0))) + expr = &TREE_OPERAND (*expr, 1); + else + break; + } + else + break; + } + + return expr; +} -/* If TT is a variable or function decl replace it with its - prevailing variant. */ +/* If TT is a variable or function decl or a simple expression containing one, + replace the occurrence with its prevailing variant. This should be able to + deal with all the expressions attached to _DECL and _TYPE nodes which were + streamed into the GIMPLE IR. */ #define LTO_SET_PREVAIL(tt) \ do {\ + tree *loc; \ if ((tt) && VAR_OR_FUNCTION_DECL_P (tt) \ && (TREE_PUBLIC (tt) || DECL_EXTERNAL (tt))) \ { \ - tt = lto_symtab_prevailing_decl (tt); \ + tt = lto_symtab_prevailing_decl (tt); \ + fixed = true; \ + } \ + else if ((tt) && EXPR_P (tt) \ + && (loc = walk_simple_constant_arithmetic (&tt)) \ + && VAR_OR_FUNCTION_DECL_P (*loc) \ + && (TREE_PUBLIC (*loc) || DECL_EXTERNAL (*loc))) \ + { \ + *loc = lto_symtab_prevailing_decl (*loc); \ fixed = true; \ } \ } while (0)