* [PATCH] Fix PR57993 (ICE in slsr)
@ 2013-07-28 0:54 Bill Schmidt
2013-07-29 19:03 ` Jeff Law
0 siblings, 1 reply; 2+ messages in thread
From: Bill Schmidt @ 2013-07-28 0:54 UTC (permalink / raw)
To: gcc-patches; +Cc: jakub
PR57993 reports a scenario where a conditional candidate is incorrectly
replaced when its putative "hidden basis" (the basis hidden by one or
more PHI nodes) does not dominate all of the PHI nodes on which the
candidate depends.
This indicates that the hidden basis was incorrectly identified as a
useful basis for the candidate. There are two ways we can fix this.
One would be to add more complexity to the code that determines the
hidden basis. Currently that code does not recursively follow the PHI
definitions backwards to ensure that the basis dominates all the PHIs.
Adding code to do this would ensure we didn't identify an incorrect
basis in the first place, but would duplicate quite a bit of existing
logic in the later analysis phase, and increase compile time.
What I've done here is to instead delay detection of the problem until
the analysis phase. If we detect that we chose an invalid basis, we
assign an "infinite" cost to the replacement so that we won't pursue it
further.
This requires that we know which basic block contains the basis
statement. The basis statement may have itself been replaced by another
statement earlier, so we need to keep the candidate table up to date
with respect to such replacements.
Bootstrapped and tested on powerpc64-linux-gnu with no new regressions.
Ok for trunk?
Thanks,
Bill
gcc:
2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com>
* gimple-ssa-strength-reduction.c (replace_mult_candidate): Record
replaced statement in the candidate table.
(phi_add_costs): Return infinite cost when the hidden basis does
not dominate all phis on which the candidate is dependent.
(replace_one_candidate): Record replaced statement in the
candidate table.
gcc/testsuite:
2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com>
* gcc.dg/torture/pr57993.c: New test.
Index: gcc/testsuite/gcc.dg/torture/pr57993.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57993.c (revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57993.c (revision 0)
@@ -0,0 +1,30 @@
+/* This ICEd prior to fixing PR57993. */
+/* { dg-do compile } */
+
+int a, b, c, d;
+char e;
+unsigned g;
+
+void f(void)
+{
+ int h;
+
+ for(; d; d++)
+ if(d)
+lbl:
+ g + a || (d = 0);
+
+ b && (a = e);
+
+ for(h = 0; h < 1; ++h)
+ {
+ h = c ? : (d = 0);
+ g = a = (e | 0);
+ }
+
+ if(a)
+ goto lbl;
+
+ a = e = 0;
+ goto lbl;
+}
Index: gcc/gimple-ssa-strength-reduction.c
===================================================================
--- gcc/gimple-ssa-strength-reduction.c (revision 201267)
+++ gcc/gimple-ssa-strength-reduction.c (working copy)
@@ -1882,6 +1882,7 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt);
gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
gsi_replace (&gsi, copy_stmt, false);
+ c->cand_stmt = copy_stmt;
if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = copy_stmt;
}
@@ -2179,6 +2180,18 @@ phi_add_costs (gimple phi, slsr_cand_t c, int one_
int cost = 0;
slsr_cand_t phi_cand = base_cand_from_table (gimple_phi_result (phi));
+ /* If we work our way back to a phi that isn't dominated by the hidden
+ basis, this isn't a candidate for replacement. Indicate this by
+ returning an unreasonably high cost. It's not easy to detect
+ these situations when determining the basis, so we defer the
+ decision until now. */
+ basic_block phi_bb = gimple_bb (phi);
+ slsr_cand_t basis = lookup_cand (c->basis);
+ basic_block basis_bb = gimple_bb (basis->cand_stmt);
+
+ if (phi_bb == basis_bb || !dominated_by_p (CDI_DOMINATORS, phi_bb, basis_bb))
+ return COST_INFINITE;
+
for (i = 0; i < gimple_phi_num_args (phi); i++)
{
tree arg = gimple_phi_arg_def (phi, i);
@@ -3226,6 +3239,7 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt);
gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
gsi_replace (&gsi, copy_stmt, false);
+ c->cand_stmt = copy_stmt;
if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = copy_stmt;
@@ -3238,6 +3252,7 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
NULL_TREE);
gimple_set_location (cast_stmt, gimple_location (c->cand_stmt));
gsi_replace (&gsi, cast_stmt, false);
+ c->cand_stmt = cast_stmt;
if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = cast_stmt;
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] Fix PR57993 (ICE in slsr)
2013-07-28 0:54 [PATCH] Fix PR57993 (ICE in slsr) Bill Schmidt
@ 2013-07-29 19:03 ` Jeff Law
0 siblings, 0 replies; 2+ messages in thread
From: Jeff Law @ 2013-07-29 19:03 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, jakub
On 07/27/2013 06:51 PM, Bill Schmidt wrote:
> PR57993 reports a scenario where a conditional candidate is incorrectly
> replaced when its putative "hidden basis" (the basis hidden by one or
> more PHI nodes) does not dominate all of the PHI nodes on which the
> candidate depends.
>
> This indicates that the hidden basis was incorrectly identified as a
> useful basis for the candidate. There are two ways we can fix this.
> One would be to add more complexity to the code that determines the
> hidden basis. Currently that code does not recursively follow the PHI
> definitions backwards to ensure that the basis dominates all the PHIs.
> Adding code to do this would ensure we didn't identify an incorrect
> basis in the first place, but would duplicate quite a bit of existing
> logic in the later analysis phase, and increase compile time.
>
> What I've done here is to instead delay detection of the problem until
> the analysis phase. If we detect that we chose an invalid basis, we
> assign an "infinite" cost to the replacement so that we won't pursue it
> further.
>
> This requires that we know which basic block contains the basis
> statement. The basis statement may have itself been replaced by another
> statement earlier, so we need to keep the candidate table up to date
> with respect to such replacements.
>
> Bootstrapped and tested on powerpc64-linux-gnu with no new regressions.
> Ok for trunk?
>
> Thanks,
> Bill
>
>
> gcc:
>
> 2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com>
>
> * gimple-ssa-strength-reduction.c (replace_mult_candidate): Record
> replaced statement in the candidate table.
> (phi_add_costs): Return infinite cost when the hidden basis does
> not dominate all phis on which the candidate is dependent.
> (replace_one_candidate): Record replaced statement in the
> candidate table.
>
> gcc/testsuite:
>
> 2013-07-27 Bill Schmidt <wschmidt@vnet.linux.ibm.com>
>
> * gcc.dg/torture/pr57993.c: New test.
Looks good. Please install!
Thanks,
Jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-07-29 18:46 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-28 0:54 [PATCH] Fix PR57993 (ICE in slsr) Bill Schmidt
2013-07-29 19:03 ` Jeff Law
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).