| Message ID | 20150522121552.GC91616@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, 22 May 2015, Jan Hubicka wrote: > Hi, > this patch fixes few cases where we compute alias type and don't need to > that are found by adding type_with_alias_set_p check to alias.c (I will send > this patch separately as there is still one ICE caught by it I believe > originating from ipa-icf-gimple, I have more involved fix for that) > > The point of all this is to check that we don't make bogus querries to alias > oracle and don't try to use them somehow - we did in ipa-icf-gimple. This is a > bug as the alias oracle gives stupid answers to stupid questions and sometimes > think the types conflicts sometimes thinks they doesn't. The other three cases > are sort of just pointless computations. > > Next part I would like to break out is the path computing equivalences considering > incomplete types (i.e. where all strucutres are equivalent and so all unions or > all functions of a given return value) > > Bootstrapped/regtested x86_64-linux, OK? > > Honza > > * tree-streamer-out.c (pack_ts_type_common_value_fields): Only consider > alias set for types that have them. > * lto-streamer-out.c (DFS::DFS_write_tree_body): Likewise. > * emit-rtl.c (set_mem_attributes_minus_bitpos): Likewise. > * ipa-icf-gimple.c (func_checker::compatible_types_p): Likewise. > Index: tree-streamer-out.c > =================================================================== > --- tree-streamer-out.c (revision 223508) > +++ tree-streamer-out.c (working copy) > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct > alias-set zero to this type. */ > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 > || (!in_lto_p > + && type_with_alias_set_p (expr) > && get_alias_set (expr) == 0)) ? 0 : -1); > } > > Index: lto-streamer-out.c > =================================================================== > --- lto-streamer-out.c (revision 223508) > +++ lto-streamer-out.c (working copy) > @@ -1146,6 +1146,7 @@ hash_tree (struct streamer_tree_cache_d > hstate.add_int (TYPE_ALIGN (t)); > hstate.add_int ((TYPE_ALIAS_SET (t) == 0 > || (!in_lto_p > + && type_with_alias_set_p (t) > && get_alias_set (t) == 0)) > ? 0 : -1); > } > Index: emit-rtl.c > =================================================================== > --- emit-rtl.c (revision 223508) > +++ emit-rtl.c (working copy) > @@ -1787,8 +1787,15 @@ set_mem_attributes_minus_bitpos (rtx ref > memset (&attrs, 0, sizeof (attrs)); > > /* Get the alias set from the expression or type (perhaps using a > - front-end routine) and use it. */ > - attrs.alias = get_alias_set (t); > + front-end routine) and use it. > + > + We may be called to produce MEM RTX for variable of incomplete type. > + This MEM RTX will only be used to produce address of a vairable, so > + we do not need to compute alias set. */ But why do we set mem_attributes for it then? > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > + attrs.alias = get_alias_set (t); > + else > + gcc_assert (DECL_P (t) && TREE_ADDRESSABLE (t)); This assert also looks fishy to me... (also just use 'type' instead of TREE_TYPE (t), not sure why you need to pun to the main variant here either - get_alias_set will do that for you and so should type_with_alias_set_p if that is necessary). The rest of the patch looks ok. Thanks, Richard. > MEM_VOLATILE_P (ref) |= TYPE_VOLATILE (type); > MEM_POINTER (ref) = POINTER_TYPE_P (type); > Index: ipa-icf-gimple.c > =================================================================== > --- ipa-icf-gimple.c (revision 223508) > +++ ipa-icf-gimple.c (working copy) > @@ -274,6 +274,12 @@ func_checker::compatible_types_p (tree t > if (!types_compatible_p (t1, t2)) > return return_false_with_msg ("types are not compatible"); > > + /* FIXME: type compatiblity checks for types that are never stored > + to memory is not very useful. We should update the code to avoid > + calling compatible_types_p on types that will never be used. */ > + if (!type_with_alias_set_p (t1) || !type_with_alias_set_p (t2)) > + return true; > + > if (get_alias_set (t1) != get_alias_set (t2)) > return return_false_with_msg ("alias sets are different"); > > >
> > Index: emit-rtl.c > > =================================================================== > > --- emit-rtl.c (revision 223508) > > +++ emit-rtl.c (working copy) > > @@ -1787,8 +1787,15 @@ set_mem_attributes_minus_bitpos (rtx ref > > memset (&attrs, 0, sizeof (attrs)); > > > > /* Get the alias set from the expression or type (perhaps using a > > - front-end routine) and use it. */ > > - attrs.alias = get_alias_set (t); > > + front-end routine) and use it. > > + > > + We may be called to produce MEM RTX for variable of incomplete type. > > + This MEM RTX will only be used to produce address of a vairable, so > > + we do not need to compute alias set. */ > > But why do we set mem_attributes for it then? Because we are stupid. We want to produce &var, where var is of incomplete type and to do it, we need to compute DECL_RTL of var and that is MEM RTX and we assign attributes to every DECL_RTL MEM we produce. I do not see how to cut this down except for having something like DECL_RTL_ADDR and have way to build it without actually making MEM expr... (which would save some memory for the MEM RTX and for attributes but it would make it difficult to hook that value somewhere) > > > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > > + attrs.alias = get_alias_set (t); > > + else > > + gcc_assert (DECL_P (t) && TREE_ADDRESSABLE (t)); > > This assert also looks fishy to me... (also just use 'type' instead > of TREE_TYPE (t), not sure why you need to pun to the main variant > here either - get_alias_set will do that for you and so should > type_with_alias_set_p if that is necessary). I am not sure if TYPE_MAIN_VARIANT is really needed here. What I know is that complete types may have incomplete variants. I was bit worried that I can disable calculation of alias set of something that do matter by not checking main variant in case we somehow manage to have a variable of incomplete vairant type but still access it because we know its complete main variant (that would be somewhat broken). That is also why I added the check - the idea is that at least I can check that this particular var is indeed having address taken. Other option would be to define an invalid alias set and assign MEM RTX this invalid alias set and make alias.c bomb when it trips across it. I actually noticed it triggers during producing some libjava glue where we forgot to set TREE_ADDRESSABLE for artificial variable that takes address. I will look into fix later today or tomorrow. Honza > > The rest of the patch looks ok. > > Thanks, > Richard. > > > MEM_VOLATILE_P (ref) |= TYPE_VOLATILE (type); > > MEM_POINTER (ref) = POINTER_TYPE_P (type); > > Index: ipa-icf-gimple.c > > =================================================================== > > --- ipa-icf-gimple.c (revision 223508) > > +++ ipa-icf-gimple.c (working copy) > > @@ -274,6 +274,12 @@ func_checker::compatible_types_p (tree t > > if (!types_compatible_p (t1, t2)) > > return return_false_with_msg ("types are not compatible"); > > > > + /* FIXME: type compatiblity checks for types that are never stored > > + to memory is not very useful. We should update the code to avoid > > + calling compatible_types_p on types that will never be used. */ > > + if (!type_with_alias_set_p (t1) || !type_with_alias_set_p (t2)) > > + return true; > > + > > if (get_alias_set (t1) != get_alias_set (t2)) > > return return_false_with_msg ("alias sets are different"); > > > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
On Fri, 22 May 2015, Jan Hubicka wrote: > > > Index: emit-rtl.c > > > =================================================================== > > > --- emit-rtl.c (revision 223508) > > > +++ emit-rtl.c (working copy) > > > @@ -1787,8 +1787,15 @@ set_mem_attributes_minus_bitpos (rtx ref > > > memset (&attrs, 0, sizeof (attrs)); > > > > > > /* Get the alias set from the expression or type (perhaps using a > > > - front-end routine) and use it. */ > > > - attrs.alias = get_alias_set (t); > > > + front-end routine) and use it. > > > + > > > + We may be called to produce MEM RTX for variable of incomplete type. > > > + This MEM RTX will only be used to produce address of a vairable, so > > > + we do not need to compute alias set. */ > > > > But why do we set mem_attributes for it then? > > Because we are stupid. We want to produce &var, where var is of incomplete type > and to do it, we need to compute DECL_RTL of var and that is MEM RTX and we assign > attributes to every DECL_RTL MEM we produce. I do not see how to cut this down > except for having something like DECL_RTL_ADDR and have way to build it without > actually making MEM expr... (which would save some memory for the MEM RTX and for > attributes but it would make it difficult to hook that value somewhere) Ok, I see... > > > > > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > > > + attrs.alias = get_alias_set (t); > > > + else > > > + gcc_assert (DECL_P (t) && TREE_ADDRESSABLE (t)); > > > > This assert also looks fishy to me... (also just use 'type' instead > > of TREE_TYPE (t), not sure why you need to pun to the main variant > > here either - get_alias_set will do that for you and so should > > type_with_alias_set_p if that is necessary). > > I am not sure if TYPE_MAIN_VARIANT is really needed here. What I know is that > complete types may have incomplete variants. How can that be? TYPE_FIELDS is shared across variants and all variants should be layed out. > I was bit worried that I can > disable calculation of alias set of something that do matter by not checking > main variant in case we somehow manage to have a variable of incomplete vairant > type but still access it because we know its complete main variant (that would > be somewhat broken). > > That is also why I added the check - the idea is that at least I can check that this > particular var is indeed having address taken. Other option would be to define > an invalid alias set and assign MEM RTX this invalid alias set and make alias.c > bomb when it trips across it. > > I actually noticed it triggers during producing some libjava glue where we > forgot to set TREE_ADDRESSABLE for artificial variable that takes address. > I will look into fix later today or tomorrow. Ok, thanks. Richard. > Honza > > > > The rest of the patch looks ok. > > > > Thanks, > > Richard. > > > > > MEM_VOLATILE_P (ref) |= TYPE_VOLATILE (type); > > > MEM_POINTER (ref) = POINTER_TYPE_P (type); > > > Index: ipa-icf-gimple.c > > > =================================================================== > > > --- ipa-icf-gimple.c (revision 223508) > > > +++ ipa-icf-gimple.c (working copy) > > > @@ -274,6 +274,12 @@ func_checker::compatible_types_p (tree t > > > if (!types_compatible_p (t1, t2)) > > > return return_false_with_msg ("types are not compatible"); > > > > > > + /* FIXME: type compatiblity checks for types that are never stored > > > + to memory is not very useful. We should update the code to avoid > > > + calling compatible_types_p on types that will never be used. */ > > > + if (!type_with_alias_set_p (t1) || !type_with_alias_set_p (t2)) > > > + return true; > > > + > > > if (get_alias_set (t1) != get_alias_set (t2)) > > > return return_false_with_msg ("alias sets are different"); > > > > > > > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) > >
Hi, On Fri, 22 May 2015, Jan Hubicka wrote: > Index: tree-streamer-out.c > =================================================================== > --- tree-streamer-out.c (revision 223508) > +++ tree-streamer-out.c (working copy) > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct > alias-set zero to this type. */ > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 > || (!in_lto_p > + && type_with_alias_set_p (expr) > && get_alias_set (expr) == 0)) ? 0 : -1); I find such interfaces very ugly. IOW, when it's always (or often) necessary to call check_foo_p() before foo() can be called then the checking should be part of foo() (and it should then return a conservative value, i.e. alias set 0), and that requirement not be imposed on the callers of foo(). I.e. why can't whatever checks you do in type_with_alias_set_p be included in get_alias_set? > + front-end routine) and use it. > + > + We may be called to produce MEM RTX for variable of incomplete type. > + This MEM RTX will only be used to produce address of a vairable, so > + we do not need to compute alias set. */ > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > + attrs.alias = get_alias_set (t); And if the checking needs to go down the main-variant chain then this should be done inside type_with_alias_set_p(), not in the caller, otherwise even the symmetry between arguments of type_with_alias_set_p(xy) and get_alias_set(xy) is destroyed (but see above for why I think type_with_alias_set_p shouldn't even exist). Ciao, Michael.
> Hi, > > On Fri, 22 May 2015, Jan Hubicka wrote: > > > Index: tree-streamer-out.c > > =================================================================== > > --- tree-streamer-out.c (revision 223508) > > +++ tree-streamer-out.c (working copy) > > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct > > alias-set zero to this type. */ > > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 > > || (!in_lto_p > > + && type_with_alias_set_p (expr) > > && get_alias_set (expr) == 0)) ? 0 : -1); > > I find such interfaces very ugly. IOW, when it's always (or often) > necessary to call check_foo_p() before foo() can be called then the > checking should be part of foo() (and it should then return a conservative > value, i.e. alias set 0), and that requirement not be imposed on the > callers of foo(). I.e. why can't whatever checks you do in > type_with_alias_set_p be included in get_alias_set? Because of sanity checking: I want to make alias sets of those types undefined rather than having random values. The point is that using the alias set in alias oracle querry is wrong. Now I run into the case that we do produce MEM exprs for incomplete variants just to take their address so I was thinking the other day about defining an invalid alias set -2, making get_alias_set to return it and ICE later when query is actually made? We do have wrong query problems at least in ipa-icf, so I think it is worthwhile sanity check. > > > + front-end routine) and use it. > > + > > + We may be called to produce MEM RTX for variable of incomplete type. > > + This MEM RTX will only be used to produce address of a vairable, so > > + we do not need to compute alias set. */ > > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > > + attrs.alias = get_alias_set (t); > > And if the checking needs to go down the main-variant chain then this > should be done inside type_with_alias_set_p(), not in the caller, > otherwise even the symmetry between arguments of type_with_alias_set_p(xy) > and get_alias_set(xy) is destroyed (but see above for why I think > type_with_alias_set_p shouldn't even exist). Yep, good point - I will cleanup this. Honza
> > > > I am not sure if TYPE_MAIN_VARIANT is really needed here. What I know is that > > complete types may have incomplete variants. > > How can that be? TYPE_FIELDS is shared across variants and all variants > should be layed out. Because TYPE_FILEDS are not always shared across variants. For example gfc_nonrestricted_type builds variants of types that have their own TYPE_FIELDS lists whose types are variants of the original TYPE_FIELDs. C++ FE used to do the same for member pointers, but I noticed that last stage1 with early version of type verifier and as far as I can remember Jason changed that. Honza
> > > > > > I am not sure if TYPE_MAIN_VARIANT is really needed here. What I know is that > > > complete types may have incomplete variants. > > > > How can that be? TYPE_FIELDS is shared across variants and all variants > > should be layed out. > > Because TYPE_FILEDS are not always shared across variants. For example > gfc_nonrestricted_type builds variants of types that have their own TYPE_FIELDS > lists whose types are variants of the original TYPE_FIELDs. C++ FE used to do > the same for member pointers, but I noticed that last stage1 with early version > of type verifier and as far as I can remember Jason changed that. And also I am positive that we do have incomplete variants of complete types in IL. Those seem to happen for C++ templates. I can dig more into reason for these appearing. We also also get wrong self-referring builtin types. This we discussed in https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00562.html gcov_info_type still fails the type verifier because we produce variant that has TYPE_SIZE but no TYPE_FIELDS. It does not happen on mainline only becuase we moved verify_type into dwarf2out and that type never gets into debug info. I don't see how to fix this other way than by my original patch in the thread above - upon completing builtin type we should copy its fields from main variant at the same time we copy size. FEs are doing that. Honza > > Honza
On Tue, 26 May 2015, Jan Hubicka wrote: > > Hi, > > > > On Fri, 22 May 2015, Jan Hubicka wrote: > > > > > Index: tree-streamer-out.c > > > =================================================================== > > > --- tree-streamer-out.c (revision 223508) > > > +++ tree-streamer-out.c (working copy) > > > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct > > > alias-set zero to this type. */ > > > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 > > > || (!in_lto_p > > > + && type_with_alias_set_p (expr) > > > && get_alias_set (expr) == 0)) ? 0 : -1); > > > > I find such interfaces very ugly. IOW, when it's always (or often) > > necessary to call check_foo_p() before foo() can be called then the > > checking should be part of foo() (and it should then return a conservative > > value, i.e. alias set 0), and that requirement not be imposed on the > > callers of foo(). I.e. why can't whatever checks you do in > > type_with_alias_set_p be included in get_alias_set? > > Because of sanity checking: I want to make alias sets of those types undefined > rather than having random values. The point is that using the alias set in > alias oracle querry is wrong. You could have just returned 0 for the alias-set for !type_with_alias_set_p in get_alias_set. That avoids polluting the alias data structures and is neither random or wrong. > Now I run into the case that we do produce MEM exprs for incomplete variants > just to take their address so I was thinking the other day about defining an > invalid alias set -2, making get_alias_set to return it and ICE later when query > is actually made? > > We do have wrong query problems at least in ipa-icf, so I think it is worthwhile > sanity check. > > > > > + front-end routine) and use it. > > > + > > > + We may be called to produce MEM RTX for variable of incomplete type. > > > + This MEM RTX will only be used to produce address of a vairable, so > > > + we do not need to compute alias set. */ > > > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) > > > + attrs.alias = get_alias_set (t); > > > > And if the checking needs to go down the main-variant chain then this > > should be done inside type_with_alias_set_p(), not in the caller, > > otherwise even the symmetry between arguments of type_with_alias_set_p(xy) > > and get_alias_set(xy) is destroyed (but see above for why I think > > type_with_alias_set_p shouldn't even exist). > > Yep, good point - I will cleanup this. > > Honza > >
On Wed, 27 May 2015, Jan Hubicka wrote: > > > > > > I am not sure if TYPE_MAIN_VARIANT is really needed here. What I know is that > > > complete types may have incomplete variants. > > > > How can that be? TYPE_FIELDS is shared across variants and all variants > > should be layed out. > > Because TYPE_FILEDS are not always shared across variants. For example > gfc_nonrestricted_type builds variants of types that have their own > TYPE_FIELDS lists whose types are variants of the original TYPE_FIELDs. > C++ FE used to do the same for member pointers, but I noticed that last > stage1 with early version of type verifier and as far as I can remember > Jason changed that. The fortran one needs to be "fixed" to use the new MEM_REF restrict support. Richard.
> On Tue, 26 May 2015, Jan Hubicka wrote: > > > > Hi, > > > > > > On Fri, 22 May 2015, Jan Hubicka wrote: > > > > > > > Index: tree-streamer-out.c > > > > =================================================================== > > > > --- tree-streamer-out.c (revision 223508) > > > > +++ tree-streamer-out.c (working copy) > > > > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct > > > > alias-set zero to this type. */ > > > > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 > > > > || (!in_lto_p > > > > + && type_with_alias_set_p (expr) > > > > && get_alias_set (expr) == 0)) ? 0 : -1); > > > > > > I find such interfaces very ugly. IOW, when it's always (or often) > > > necessary to call check_foo_p() before foo() can be called then the > > > checking should be part of foo() (and it should then return a conservative > > > value, i.e. alias set 0), and that requirement not be imposed on the > > > callers of foo(). I.e. why can't whatever checks you do in > > > type_with_alias_set_p be included in get_alias_set? > > > > Because of sanity checking: I want to make alias sets of those types undefined > > rather than having random values. The point is that using the alias set in > > alias oracle querry is wrong. > > You could have just returned 0 for the alias-set for > !type_with_alias_set_p in get_alias_set. That avoids polluting the > alias data structures and is neither random or wrong. Take the example of bug in ipa-ICF. It is digging out completely random types from the IL and thinks it absolutely must compare alias sets of all of them (the bug obviously is that it really should compare only those that matters). It then throws random incomplete type to get_alias_set and obtain 0. Which will make it to silently give up if the "matching" random type is complete. ICE here is a friendly reminder to the author of the optimization pass that he is doing something fishy. It will also catch the cases where we throw memory access of incomplete type to the function body by frontend/middleend bug instead of just silently disabling optimization. I caught the Java interface glue issue with this. (still need to fix that) Now pack_ts_type_common_value_fields and RTL generation are differnt from the usual use of alias set oracle in a sense that they do compute unnecesary alias sets by design. They are not optimizations, they are IL stage transitions. Honza
Index: tree-streamer-out.c =================================================================== --- tree-streamer-out.c (revision 223508) +++ tree-streamer-out.c (working copy) @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct alias-set zero to this type. */ bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0 || (!in_lto_p + && type_with_alias_set_p (expr) && get_alias_set (expr) == 0)) ? 0 : -1); } Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 223508) +++ lto-streamer-out.c (working copy) @@ -1146,6 +1146,7 @@ hash_tree (struct streamer_tree_cache_d hstate.add_int (TYPE_ALIGN (t)); hstate.add_int ((TYPE_ALIAS_SET (t) == 0 || (!in_lto_p + && type_with_alias_set_p (t) && get_alias_set (t) == 0)) ? 0 : -1); } Index: emit-rtl.c =================================================================== --- emit-rtl.c (revision 223508) +++ emit-rtl.c (working copy) @@ -1787,8 +1787,15 @@ set_mem_attributes_minus_bitpos (rtx ref memset (&attrs, 0, sizeof (attrs)); /* Get the alias set from the expression or type (perhaps using a - front-end routine) and use it. */ - attrs.alias = get_alias_set (t); + front-end routine) and use it. + + We may be called to produce MEM RTX for variable of incomplete type. + This MEM RTX will only be used to produce address of a vairable, so + we do not need to compute alias set. */ + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t)))) + attrs.alias = get_alias_set (t); + else + gcc_assert (DECL_P (t) && TREE_ADDRESSABLE (t)); MEM_VOLATILE_P (ref) |= TYPE_VOLATILE (type); MEM_POINTER (ref) = POINTER_TYPE_P (type); Index: ipa-icf-gimple.c =================================================================== --- ipa-icf-gimple.c (revision 223508) +++ ipa-icf-gimple.c (working copy) @@ -274,6 +274,12 @@ func_checker::compatible_types_p (tree t if (!types_compatible_p (t1, t2)) return return_false_with_msg ("types are not compatible"); + /* FIXME: type compatiblity checks for types that are never stored + to memory is not very useful. We should update the code to avoid + calling compatible_types_p on types that will never be used. */ + if (!type_with_alias_set_p (t1) || !type_with_alias_set_p (t2)) + return true; + if (get_alias_set (t1) != get_alias_set (t2)) return return_false_with_msg ("alias sets are different");