Re: [PATCH][GCC 7] Fix PR70171

[Richard Biener <richard.guenther@gmail.com>]

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 } } } } } */