[PATCH][GCC 7] Fix PR70171

[PATCH][GCC 7] Fix PR70171

[Richard Biener]

The following teaches phiprop to handle the case of aggregate copies
where the aggregate has non-BLKmode which means it is very likely
expanded as reg-reg moves (any better test for that apart from
checking for non-BLKmode?).  This improves code for the testcase
from

_Z14struct_ternary1SS_b:
.LFB2:
        .cfi_startproc
        leaq    -40(%rsp), %rcx
        leaq    -24(%rsp), %rax
        testb   %dl, %dl
        movl    %edi, -24(%rsp)
        movl    %esi, -40(%rsp)
        cmove   %rcx, %rax
        movl    (%rax), %eax
        ret

to

_Z14struct_ternary1SS_b:
.LFB2:
        .cfi_startproc
        testb   %dl, %dl
        movl    %edi, %eax
        cmove   %esi, %eax
        ret

Bootstrapped and tested on x86_64-unknown-linux-gnu, queued for stage1.

Richard.

2016-03-11  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/70171
	* tree-ssa-phiprop.c: Include stor-layout.h.
	(phiprop_insert_phi): Handle the aggregate copy case.
	(propagate_with_phi): Likewise.

	* g++.dg/tree-ssa/pr70171.C: New testcase.

Index: gcc/tree-ssa-phiprop.c
===================================================================
*** gcc/tree-ssa-phiprop.c	(revision 234134)
--- gcc/tree-ssa-phiprop.c	(working copy)
*************** along with GCC; see the file COPYING3.
*** 31,36 ****
--- 31,37 ----
  #include "tree-eh.h"
  #include "gimplify.h"
  #include "gimple-iterator.h"
+ #include "stor-layout.h"
  
  /* This pass propagates indirect loads through the PHI node for its
     address to make the load source possibly non-addressable and to
*************** phiprop_insert_phi (basic_block bb, gphi
*** 132,138 ****
  		    struct phiprop_d *phivn, size_t n)
  {
    tree res;
!   gphi *new_phi;
    edge_iterator ei;
    edge e;
  
--- 133,139 ----
  		    struct phiprop_d *phivn, size_t n)
  {
    tree res;
!   gphi *new_phi = NULL;
    edge_iterator ei;
    edge e;
  
*************** phiprop_insert_phi (basic_block bb, gphi
*** 142,148 ****
    /* Build a new PHI node to replace the definition of
       the indirect reference lhs.  */
    res = gimple_assign_lhs (use_stmt);
!   new_phi = create_phi_node (res, bb);
  
    if (dump_file && (dump_flags & TDF_DETAILS))
      {
--- 143,150 ----
    /* Build a new PHI node to replace the definition of
       the indirect reference lhs.  */
    res = gimple_assign_lhs (use_stmt);
!   if (TREE_CODE (res) == SSA_NAME)
!     new_phi = create_phi_node (res, bb);
  
    if (dump_file && (dump_flags & TDF_DETAILS))
      {
*************** phiprop_insert_phi (basic_block bb, gphi
*** 187,193 ****
  	{
  	  tree rhs = gimple_assign_rhs1 (use_stmt);
  	  gcc_assert (TREE_CODE (old_arg) == ADDR_EXPR);
! 	  new_var = make_ssa_name (TREE_TYPE (rhs));
  	  if (!is_gimple_min_invariant (old_arg))
  	    old_arg = PHI_ARG_DEF_FROM_EDGE (phi, e);
  	  else
--- 189,198 ----
  	{
  	  tree rhs = gimple_assign_rhs1 (use_stmt);
  	  gcc_assert (TREE_CODE (old_arg) == ADDR_EXPR);
! 	  if (TREE_CODE (res) == SSA_NAME)
! 	    new_var = make_ssa_name (TREE_TYPE (rhs));
! 	  else
! 	    new_var = unshare_expr (res);
  	  if (!is_gimple_min_invariant (old_arg))
  	    old_arg = PHI_ARG_DEF_FROM_EDGE (phi, e);
  	  else
*************** phiprop_insert_phi (basic_block bb, gphi
*** 210,222 ****
  	    }
  	}
  
!       add_phi_arg (new_phi, new_var, e, locus);
      }
  
!   update_stmt (new_phi);
  
!   if (dump_file && (dump_flags & TDF_DETAILS))
!     print_gimple_stmt (dump_file, new_phi, 0, 0);
  
    return res;
  }
--- 215,231 ----
  	    }
  	}
  
!       if (new_phi)
! 	add_phi_arg (new_phi, new_var, e, locus);
      }
  
!   if (new_phi)
!     {
!       update_stmt (new_phi);
  
!       if (dump_file && (dump_flags & TDF_DETAILS))
! 	print_gimple_stmt (dump_file, new_phi, 0, 0);
!     }
  
    return res;
  }
*************** propagate_with_phi (basic_block bb, gphi
*** 250,256 ****
    tree type = NULL_TREE;
  
    if (!POINTER_TYPE_P (TREE_TYPE (ptr))
!       || !is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
      return false;
  
    /* Check if we can "cheaply" dereference all phi arguments.  */
--- 259,266 ----
    tree type = NULL_TREE;
  
    if (!POINTER_TYPE_P (TREE_TYPE (ptr))
!       || (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr)))
! 	  && TYPE_MODE (TREE_TYPE (TREE_TYPE (ptr))) == BLKmode))
      return false;
  
    /* Check if we can "cheaply" dereference all phi arguments.  */
*************** propagate_with_phi (basic_block bb, gphi
*** 306,312 ****
           
        /* Check whether this is a load of *ptr.  */
        if (!(is_gimple_assign (use_stmt)
- 	    && TREE_CODE (gimple_assign_lhs (use_stmt)) == SSA_NAME
  	    && gimple_assign_rhs_code (use_stmt) == MEM_REF
  	    && TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 0) == ptr
  	    && integer_zerop (TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 1))
--- 316,321 ----
*************** propagate_with_phi (basic_block bb, gphi
*** 327,335 ****
  				  bb, gimple_bb (def_stmt))))
  	goto next;
  
        /* Found a proper dereference.  Insert a phi node if this
  	 is the first load transformation.  */
!       if (!phi_inserted)
  	{
  	  res = phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
  	  type = TREE_TYPE (res);
--- 336,366 ----
  				  bb, gimple_bb (def_stmt))))
  	goto next;
  
+       /* Found a proper dereference with an aggregate copy.  Just
+          insert aggregate copies on the edges instead.  */
+       if (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
+ 	{
+ 	  phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
+ 
+ 	  /* Remove old stmt.  The phi is taken care of by DCE.  */
+ 	  gsi = gsi_for_stmt (use_stmt);
+ 	  /* Unlinking the VDEF here is fine as we are sure that we process
+ 	     stmts in execution order due to aggregate copies having VDEFs
+ 	     and we emit loads on the edges in the very same order.
+ 	     We get multiple copies (or intermediate register loads) handled
+ 	     only by walking PHIs or immediate uses in a lucky order though,
+ 	     so we could signal the caller to re-start iterating over PHIs
+ 	     when we come here which would make it quadratic in the number
+ 	     of PHIs.  */
+ 	  unlink_stmt_vdef (use_stmt);
+ 	  gsi_remove (&gsi, true);
+ 
+ 	  phi_inserted = true;
+ 	}
+ 
        /* Found a proper dereference.  Insert a phi node if this
  	 is the first load transformation.  */
!       else if (!phi_inserted)
  	{
  	  res = phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
  	  type = TREE_TYPE (res);
Index: gcc/testsuite/g++.dg/tree-ssa/pr70171.C
===================================================================
*** gcc/testsuite/g++.dg/tree-ssa/pr70171.C	(revision 0)
--- gcc/testsuite/g++.dg/tree-ssa/pr70171.C	(working copy)
***************
*** 0 ****
--- 1,8 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2 -fdump-tree-optimized" } */
+ 
+ struct S { int i; };
+ S struct_ternary (S a, S b, bool select) { return select ? a : b; }
+ 
+ /* { dg-final { scan-tree-dump-not "&\[ab\]" "optimized" } } */
+ /* { dg-final { scan-assembler-not "\[er\]sp" { target { { i?86-*-* x86_64-*-* } && { ! ia32 } } } } } */

Re: [PATCH][GCC 7] Fix PR70171

[Eric Botcazou]

> The following teaches phiprop to handle the case of aggregate copies
> where the aggregate has non-BLKmode which means it is very likely
> expanded as reg-reg moves (any better test for that apart from
> checking for non-BLKmode?).  

!aggregate_value_p comes to mind, but non-BLKmode is the definitive test to 
distinguish the register from the non-register case at the RTL level.

-- 
Eric Botcazou

Re: [PATCH][GCC 7] Fix PR70171

[Richard Biener]

On Fri, 11 Mar 2016, Eric Botcazou wrote:

> > The following teaches phiprop to handle the case of aggregate copies
> > where the aggregate has non-BLKmode which means it is very likely
> > expanded as reg-reg moves (any better test for that apart from
> > checking for non-BLKmode?).  
> 
> !aggregate_value_p comes to mind, but non-BLKmode is the definitive test to 
> distinguish the register from the non-register case at the RTL level.

It looks like it might catch a few extra cases where the address of the
decl is required.  But it also looks like it's somewhat overly broad like

  /* Function types that are TREE_ADDRESSABLE force return in memory.  */
  if (fntype && TREE_ADDRESSABLE (fntype))
    return 1;

without actually testing 'exp' is the return slot.  In fact most of
the function cares about function return values and some about
parameters.  I guess the predicate should be split up (a quick grep
shows most callers care about the return value case).

Richard.

Re: [PATCH][GCC 7] Fix PR70171

[Eric Botcazou]

> It looks like it might catch a few extra cases where the address of the
> decl is required.  But it also looks like it's somewhat overly broad like
> 
>   /* Function types that are TREE_ADDRESSABLE force return in memory.  */
>   if (fntype && TREE_ADDRESSABLE (fntype))
>     return 1;
> 
> without actually testing 'exp' is the return slot.

I think that this particular case can only trigger in Ada. ;-)

-- 
Eric Botcazou

Re: [PATCH][GCC 7] Fix PR70171

[Richard Biener]

On Fri, Mar 11, 2016 at 3:01 PM, Richard Biener <rguenther@suse.de> wrote:
>
> The following teaches phiprop to handle the case of aggregate copies
> where the aggregate has non-BLKmode which means it is very likely
> expanded as reg-reg moves (any better test for that apart from
> checking for non-BLKmode?).  This improves code for the testcase
> from
>
> _Z14struct_ternary1SS_b:
> .LFB2:
>         .cfi_startproc
>         leaq    -40(%rsp), %rcx
>         leaq    -24(%rsp), %rax
>         testb   %dl, %dl
>         movl    %edi, -24(%rsp)
>         movl    %esi, -40(%rsp)
>         cmove   %rcx, %rax
>         movl    (%rax), %eax
>         ret
>
> to
>
> _Z14struct_ternary1SS_b:
> .LFB2:
>         .cfi_startproc
>         testb   %dl, %dl
>         movl    %edi, %eax
>         cmove   %esi, %eax
>         ret
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, queued for stage1.

Re-bootstrapped and tested on x86_64-unknown-linux-gnu, applied as r235208.

Richard.

> Richard.
>
> 2016-03-11  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/70171
>         * tree-ssa-phiprop.c: Include stor-layout.h.
>         (phiprop_insert_phi): Handle the aggregate copy case.
>         (propagate_with_phi): Likewise.
>
>         * g++.dg/tree-ssa/pr70171.C: New testcase.
>
> Index: gcc/tree-ssa-phiprop.c
> ===================================================================
> *** gcc/tree-ssa-phiprop.c      (revision 234134)
> --- gcc/tree-ssa-phiprop.c      (working copy)
> *************** along with GCC; see the file COPYING3.
> *** 31,36 ****
> --- 31,37 ----
>   #include "tree-eh.h"
>   #include "gimplify.h"
>   #include "gimple-iterator.h"
> + #include "stor-layout.h"
>
>   /* This pass propagates indirect loads through the PHI node for its
>      address to make the load source possibly non-addressable and to
> *************** phiprop_insert_phi (basic_block bb, gphi
> *** 132,138 ****
>                     struct phiprop_d *phivn, size_t n)
>   {
>     tree res;
> !   gphi *new_phi;
>     edge_iterator ei;
>     edge e;
>
> --- 133,139 ----
>                     struct phiprop_d *phivn, size_t n)
>   {
>     tree res;
> !   gphi *new_phi = NULL;
>     edge_iterator ei;
>     edge e;
>
> *************** phiprop_insert_phi (basic_block bb, gphi
> *** 142,148 ****
>     /* Build a new PHI node to replace the definition of
>        the indirect reference lhs.  */
>     res = gimple_assign_lhs (use_stmt);
> !   new_phi = create_phi_node (res, bb);
>
>     if (dump_file && (dump_flags & TDF_DETAILS))
>       {
> --- 143,150 ----
>     /* Build a new PHI node to replace the definition of
>        the indirect reference lhs.  */
>     res = gimple_assign_lhs (use_stmt);
> !   if (TREE_CODE (res) == SSA_NAME)
> !     new_phi = create_phi_node (res, bb);
>
>     if (dump_file && (dump_flags & TDF_DETAILS))
>       {
> *************** phiprop_insert_phi (basic_block bb, gphi
> *** 187,193 ****
>         {
>           tree rhs = gimple_assign_rhs1 (use_stmt);
>           gcc_assert (TREE_CODE (old_arg) == ADDR_EXPR);
> !         new_var = make_ssa_name (TREE_TYPE (rhs));
>           if (!is_gimple_min_invariant (old_arg))
>             old_arg = PHI_ARG_DEF_FROM_EDGE (phi, e);
>           else
> --- 189,198 ----
>         {
>           tree rhs = gimple_assign_rhs1 (use_stmt);
>           gcc_assert (TREE_CODE (old_arg) == ADDR_EXPR);
> !         if (TREE_CODE (res) == SSA_NAME)
> !           new_var = make_ssa_name (TREE_TYPE (rhs));
> !         else
> !           new_var = unshare_expr (res);
>           if (!is_gimple_min_invariant (old_arg))
>             old_arg = PHI_ARG_DEF_FROM_EDGE (phi, e);
>           else
> *************** phiprop_insert_phi (basic_block bb, gphi
> *** 210,222 ****
>             }
>         }
>
> !       add_phi_arg (new_phi, new_var, e, locus);
>       }
>
> !   update_stmt (new_phi);
>
> !   if (dump_file && (dump_flags & TDF_DETAILS))
> !     print_gimple_stmt (dump_file, new_phi, 0, 0);
>
>     return res;
>   }
> --- 215,231 ----
>             }
>         }
>
> !       if (new_phi)
> !       add_phi_arg (new_phi, new_var, e, locus);
>       }
>
> !   if (new_phi)
> !     {
> !       update_stmt (new_phi);
>
> !       if (dump_file && (dump_flags & TDF_DETAILS))
> !       print_gimple_stmt (dump_file, new_phi, 0, 0);
> !     }
>
>     return res;
>   }
> *************** propagate_with_phi (basic_block bb, gphi
> *** 250,256 ****
>     tree type = NULL_TREE;
>
>     if (!POINTER_TYPE_P (TREE_TYPE (ptr))
> !       || !is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
>       return false;
>
>     /* Check if we can "cheaply" dereference all phi arguments.  */
> --- 259,266 ----
>     tree type = NULL_TREE;
>
>     if (!POINTER_TYPE_P (TREE_TYPE (ptr))
> !       || (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr)))
> !         && TYPE_MODE (TREE_TYPE (TREE_TYPE (ptr))) == BLKmode))
>       return false;
>
>     /* Check if we can "cheaply" dereference all phi arguments.  */
> *************** propagate_with_phi (basic_block bb, gphi
> *** 306,312 ****
>
>         /* Check whether this is a load of *ptr.  */
>         if (!(is_gimple_assign (use_stmt)
> -           && TREE_CODE (gimple_assign_lhs (use_stmt)) == SSA_NAME
>             && gimple_assign_rhs_code (use_stmt) == MEM_REF
>             && TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 0) == ptr
>             && integer_zerop (TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 1))
> --- 316,321 ----
> *************** propagate_with_phi (basic_block bb, gphi
> *** 327,335 ****
>                                   bb, gimple_bb (def_stmt))))
>         goto next;
>
>         /* Found a proper dereference.  Insert a phi node if this
>          is the first load transformation.  */
> !       if (!phi_inserted)
>         {
>           res = phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
>           type = TREE_TYPE (res);
> --- 336,366 ----
>                                   bb, gimple_bb (def_stmt))))
>         goto next;
>
> +       /* Found a proper dereference with an aggregate copy.  Just
> +          insert aggregate copies on the edges instead.  */
> +       if (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
> +       {
> +         phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
> +
> +         /* Remove old stmt.  The phi is taken care of by DCE.  */
> +         gsi = gsi_for_stmt (use_stmt);
> +         /* Unlinking the VDEF here is fine as we are sure that we process
> +            stmts in execution order due to aggregate copies having VDEFs
> +            and we emit loads on the edges in the very same order.
> +            We get multiple copies (or intermediate register loads) handled
> +            only by walking PHIs or immediate uses in a lucky order though,
> +            so we could signal the caller to re-start iterating over PHIs
> +            when we come here which would make it quadratic in the number
> +            of PHIs.  */
> +         unlink_stmt_vdef (use_stmt);
> +         gsi_remove (&gsi, true);
> +
> +         phi_inserted = true;
> +       }
> +
>         /* Found a proper dereference.  Insert a phi node if this
>          is the first load transformation.  */
> !       else if (!phi_inserted)
>         {
>           res = phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
>           type = TREE_TYPE (res);
> Index: gcc/testsuite/g++.dg/tree-ssa/pr70171.C
> ===================================================================
> *** gcc/testsuite/g++.dg/tree-ssa/pr70171.C     (revision 0)
> --- gcc/testsuite/g++.dg/tree-ssa/pr70171.C     (working copy)
> ***************
> *** 0 ****
> --- 1,8 ----
> + /* { dg-do compile } */
> + /* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> + struct S { int i; };
> + S struct_ternary (S a, S b, bool select) { return select ? a : b; }
> +
> + /* { dg-final { scan-tree-dump-not "&\[ab\]" "optimized" } } */
> + /* { dg-final { scan-assembler-not "\[er\]sp" { target { { i?86-*-* x86_64-*-* } && { ! ia32 } } } } } */