[PR 85762, 87008, 85459] Relax MEM_REF check in contains_vce_or_bfcref_p

[Richard Biener]

On Tue, 5 Mar 2019, Martin Jambor wrote:

> Hi,
> 
> after looking into the three PRs I found that the problem is the same in
> all of them, introduced in revision 255510, with SRA treating completely
> non type-punning MEM_REFs to a filed of a structure as a V_C_E (these
> are typically introduced by inlining tiny C++ getters/setters and other
> methods), sending it to a paranoid mode and leaving many unnecessary
> memory accesses behind.
> 
> I believe the right fix is to relax the condition to what it was
> intended to do in r255510 to fix PR 83141.  This particular behavior was
> chosen because we were afraid floating-point accesses might be
> introduced to load non-FP data, but as https://pastebin.com/Jk7ZPsVH
> shows, that can still happen and so if it really must be avoided at all
> costs, we have to deal with it differently.
> 
> The regressions this fixes are potentially severe, therefore I'd like to
> backport this also to the gcc-8-branch.  The patch has passed bootstrap
> and testing on x86_64-linux and aarch64-linux, testing and bootstrap on
> i686-linux  and ppc64le-linux are in progress.
> 
> OK for trunk and then later on for the branch?
> 
> Thanks,
> 
> 
> Martin
> 
> 
> 2019-03-01  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/85762
> 	PR tree-optimization/87008
> 	PR tree-optimization/85459
> 	* tree-sra.c (contains_vce_or_bfcref_p): Relax MEM_REF type-conversion
> 	check.
> 
> 	testsuite/
> 	* g++.dg/tree-ssa/pr87008.C: New test.
> ---
>  gcc/testsuite/g++.dg/tree-ssa/pr87008.C | 17 +++++++++++++++++
>  gcc/tree-sra.c                          | 13 ++++---------
>  2 files changed, 21 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr87008.C
> 
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr87008.C b/gcc/testsuite/g++.dg/tree-ssa/pr87008.C
> new file mode 100644
> index 00000000000..eef521f9ad5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/pr87008.C
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +extern void dontcallthis();
> +
> +struct A { long a, b; };
> +struct B : A {};
> +template<class T>void cp(T&a,T const&b){a=b;}
> +long f(B x){
> +  B y; cp<A>(y,x);
> +  B z; cp<A>(z,x);
> +  if (y.a - z.a)
> +    dontcallthis ();
> +  return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "dontcallthis" "optimized" } } */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index eeef31ba496..bd30af5c6e0 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -1151,7 +1151,7 @@ contains_view_convert_expr_p (const_tree ref)
>  }
>  
>  /* Return true if REF contains a VIEW_CONVERT_EXPR or a MEM_REF that performs
> -   type conversion or a COMPONENT_REF with a bit-field field declaration.  */
> +   memcpy or a COMPONENT_REF with a bit-field field declaration.  */
>  
>  static bool
>  contains_vce_or_bfcref_p (const_tree ref)
> @@ -1165,14 +1165,9 @@ contains_vce_or_bfcref_p (const_tree ref)
>        ref = TREE_OPERAND (ref, 0);
>      }
>  
> -  if (TREE_CODE (ref) != MEM_REF
> -      || TREE_CODE (TREE_OPERAND (ref, 0)) != ADDR_EXPR)
> -    return false;
> -
> -  tree mem = TREE_OPERAND (TREE_OPERAND (ref, 0), 0);
> -  if (TYPE_MAIN_VARIANT (TREE_TYPE (ref))
> -      != TYPE_MAIN_VARIANT (TREE_TYPE (mem)))
> -    return true;
> +  if (TREE_CODE (ref) == MEM_REF
> +      && TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (ref, 1))))
> +      return true;

This doesn't make much sense to me - why not simply go back to the
old implementation which would mean to just

   return false;

here?

Richard.