[Bug tree-optimization/82224] Strict-aliasing not noticing valid aliasing of two unions with active members

["rguenth at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82224

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> One complication when deciding whether the downstream uses are fine is
> that we assign the same alias set to union accesses u->x and u->y.
> 
> That said, the union read/write pairs we remove essentially act as
> an optimization barrier - they "union" the alias sets of all components
> of the unions (not only of those accessed).  I think that's what other
> readings of the standard ask for (there's a bug about this as well).
> A mere declaration of a union needs to union the alias-sets and the
> following should be valid:
> 
> union { long l; long long ll; };
> 
> long foo (long *p)
> {
>   *p = 0;
>   *(long long *)p = 1;
>   return *p;
> }
> 
> this means (from an implementation perspective) that iff we do not
> want to go this far then accessing a union member via a non-union
> type isn't valid.
> 
> Anyway ;)  I don't see an easy way to fix the testcases in this PR
> without removing the optimization entirely.  It's currently guarded
> like
> 
>           /* If the TBAA state isn't the same for downstream reads
>              we cannot value-number the VDEFs the same.  */
>               alias_set_type set = get_alias_set (lhs);
>               if (! vnresult
>                   || vnresult->set == set
>                   || alias_set_subset_of (set, vnresult->set))
> 
> but we don't know whether vnresult is from a load or a store so we
> don't know whether it fully specifies the "TBAA state".  So even
> dumbing that down to
> 
>           if (vnresult->set != set
>               || set != get_alias_set (TREE_TYPE (lhs)))
>             resultsame = false;
> 
> where the latter test sees if the alias set for the access and for an
> access using a pointer is the same might keep some holes open.
> 
> Some of the more interesting cases _did_ involve unions and the above
> would likely make them not optimized again (I remember sth with vector
> intrinsics and initialization / copying via unions).  Oh, even more
> trivial union->x = union->x; will stop from being optimized.
> 
> Index: gcc/tree-ssa-sccvn.c
> ===================================================================
> --- gcc/tree-ssa-sccvn.c        (revision 254135)
> +++ gcc/tree-ssa-sccvn.c        (working copy)
> @@ -3800,11 +3800,11 @@ visit_reference_op_store (tree lhs, tree
>        resultsame = expressions_equal_p (result, op);
>        if (resultsame)
>         {
> -         /* If the TBAA state isn't compatible for downstream reads
> +         /* If the TBAA state isn't the same for downstream reads
>              we cannot value-number the VDEFs the same.  */
>           alias_set_type set = get_alias_set (lhs);
>           if (vnresult->set != set
> -             && ! alias_set_subset_of (set, vnresult->set))
> +             || set != get_alias_set (TREE_TYPE (lhs)))
>             resultsame = false;
>         }
>      }
> @@ -5628,9 +5628,9 @@ eliminate_dom_walker::before_dom_childre
>                  at least all accesses the later one does or if the store
>                  was to readonly memory storing the same value.  */
>               alias_set_type set = get_alias_set (lhs);
> -             if (! vnresult
> -                 || vnresult->set == set
> -                 || alias_set_subset_of (set, vnresult->set))
> +             if (vnresult
> +                 && vnresult->set == set
> +                 && set == get_alias_set (TREE_TYPE (lhs)))
>                 {
>                   if (dump_file && (dump_flags & TDF_DETAILS))
>                     {
> Index: gcc/tree-ssa-dom.c
> ===================================================================
> --- gcc/tree-ssa-dom.c  (revision 254135)
> +++ gcc/tree-ssa-dom.c  (working copy)
> @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3.
>  #include "gimplify.h"
>  #include "tree-cfgcleanup.h"
>  #include "dbgcnt.h"
> +#include "alias.h"
>  
>  /* This file implements optimizations on the dominator tree.  */
>  
> @@ -1953,7 +1954,8 @@ dom_opt_dom_walker::optimize_stmt (basic
>           gimple_set_vuse (new_stmt, gimple_vuse (stmt));
>           cached_lhs = m_avail_exprs_stack->lookup_avail_expr (new_stmt,
> false,
>                                                                false);
> -         if (cached_lhs && operand_equal_p (rhs, cached_lhs, 0))
> +         if (cached_lhs && operand_equal_p (rhs, cached_lhs, 0)
> +             && get_alias_set (lhs) == get_alias_set (TREE_TYPE (lhs)))
>             {
>               basic_block bb = gimple_bb (stmt);
>               unlink_stmt_vdef (stmt);
> 
> FAILs at least gcc.dg/tree-ssa/ssa-pre-26.c

And

FAIL: gnat.dg/opt39.adb scan-tree-dump-times optimized "MEM" 1 (found 9 times)

otherwise clean on x86_64-unknown-linux-gnu.

> as said I'm not 100% convinced the more strict handling avoids all of the
> cases.