* [PATCH][GCC 7] Fix PR70171
@ 2016-03-11 14:01 Richard Biener
2016-03-11 20:59 ` Eric Botcazou
2016-04-19 14:05 ` Richard Biener
0 siblings, 2 replies; 5+ messages in thread
From: Richard Biener @ 2016-03-11 14:01 UTC (permalink / raw)
To: gcc-patches
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 } } } } } */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][GCC 7] Fix PR70171
2016-03-11 14:01 [PATCH][GCC 7] Fix PR70171 Richard Biener
@ 2016-03-11 20:59 ` Eric Botcazou
2016-03-14 8:56 ` Richard Biener
2016-04-19 14:05 ` Richard Biener
1 sibling, 1 reply; 5+ messages in thread
From: Eric Botcazou @ 2016-03-11 20:59 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][GCC 7] Fix PR70171
2016-03-11 20:59 ` Eric Botcazou
@ 2016-03-14 8:56 ` Richard Biener
2016-03-15 10:37 ` Eric Botcazou
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2016-03-14 8:56 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][GCC 7] Fix PR70171
2016-03-14 8:56 ` Richard Biener
@ 2016-03-15 10:37 ` Eric Botcazou
0 siblings, 0 replies; 5+ messages in thread
From: Eric Botcazou @ 2016-03-15 10:37 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][GCC 7] Fix PR70171
2016-03-11 14:01 [PATCH][GCC 7] Fix PR70171 Richard Biener
2016-03-11 20:59 ` Eric Botcazou
@ 2016-04-19 14:05 ` Richard Biener
1 sibling, 0 replies; 5+ messages in thread
From: Richard Biener @ 2016-04-19 14:05 UTC (permalink / raw)
To: GCC Patches
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 } } } } } */
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-04-19 14:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-11 14:01 [PATCH][GCC 7] Fix PR70171 Richard Biener
2016-03-11 20:59 ` Eric Botcazou
2016-03-14 8:56 ` Richard Biener
2016-03-15 10:37 ` Eric Botcazou
2016-04-19 14:05 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).