Re: [PATCH] Fix PR80158

[Bill Schmidt]

On Mar 24, 2017, at 3:19 AM, Richard Biener  wrote:
> 
> On Thu, Mar 23, 2017 at 7:20 PM, Bill Schmidt
>  wrote:
>> Hi,
>> 
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80158 reports an ICE in
>> SLSR while building 416.gamess on x86_64.  This is a latent (but
>> previously harmless) bug that was exposed by the fix for PR80054.
>> When replacing any statement with a strength reduction, SLSR updates
>> the candidate statement S associated with the associated candidate
>> record in the candidate table.  However, S may actually be associated
>> with two candidate records when an expression may be treated as
>> either a CAND_ADD or a CAND_MULT.  In this case, the second one can
>> be reached via the next_interp field.
> 
> Hmm, the following code in SLSR suggests there may be more than
> one such alternate candidate:
> 
>  while (arg_cand->kind != CAND_ADD && arg_cand->kind != CAND_PHI)
>{
>  if (!arg_cand->next_interp)
>return;
> 
>  arg_cand = lookup_cand (arg_cand->next_interp);
>}

You're right, although with the current design there is never more than one
alternate interpretation.  So the patch is correct now, but I should make it 
handle
additional alternate interpretations to future-proof it.  I'll fix that later 
this week.
Thanks for catching this.

>> In the excerpt from 416.gamess, the first candidate record was
>> updated when its statement was replaced, but the next-interp record
>> was not.  Later, that record was used as a basis for another tree
>> of strength-reduction candidates, but since it now pointed to an
>> orphaned statement without a basic block, the ICE occurred when
>> checking dominance against the statement's block.
>> 
>> The fix is simply to ensure that the next_interp record is always
>> updated when present.
>> 
>> I've bootstrapped and tested this on powerpc64le-unknown-linux-gnu
>> with no regressions.  I tested that the standalone test works on
>> x86_64 with this fix, but I have been unable to perform an x86_64
>> regstrap because the machine I have access to has insufficient
>> disk space. :/  I would appreciate it if you could do this for me.
>> 
>> Assuming no problems with x86_64 regstrap, is this ok for trunk?
>> 
>> Note: I will be unreachable from now until next Tuesday (i.e.,
>> returning the 28th).  If needed, please feel free to commit the
>> patch on my behalf.  Otherwise I will attend to it when I return.
> 
> I'll do a regstrap and gamess build and will commit if that succeeds.

Thank you, I appreciate you handling this for me in my absence!

Bill

> 
> The above can be addressed as followup.
> 
> Thanks,
> Richard.
> 
>> Thanks!
>> Bill
>> 
>> 
>> [gcc]
>> 
>> 2017-03-23  Bill Schmidt  
>> 
>>PR tree-optimization/80158
>>* gimple-ssa-strength-reduction.c (replace_mult_candidate): When
>>replacing a candidate statement, also replace it for the
>>candidate's alternate interpretation.
>>(replace_rhs_if_not_dup): Likewise.
>>(replace_one_candidate): Likewise.
>> 
>> [gcc/testsuite]
>> 
>> 2017-03-23  Bill Schmidt  
>> 
>>PR tree-optimization/80158
>>* gfortran.fortran-torture/compile/pr80158.f: New file.
>> 
>> 
>> Index: gcc/gimple-ssa-strength-reduction.c
>> ===
>> --- gcc/gimple-ssa-strength-reduction.c (revision 246420)
>> +++ gcc/gimple-ssa-strength-reduction.c (working copy)
>> @@ -2089,6 +2089,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
>>  gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
>>  gsi_replace (, copy_stmt, false);
>>  c->cand_stmt = copy_stmt;
>> + if (c->next_interp)
>> +   lookup_cand (c->next_interp)->cand_stmt = copy_stmt;
>>  if (dump_file && (dump_flags & TDF_DETAILS))
>>stmt_to_print = copy_stmt;
>>}
>> @@ -2118,6 +2120,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
>>  basis_name, bump_tree);
>>  update_stmt (gsi_stmt (gsi));
>>   c->cand_stmt = gsi_stmt (gsi);
>> + if (c->next_interp)
>> +   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
>>  if (dump_file && (dump_flags & TDF_DETAILS))
>>stmt_to_print = gsi_stmt (gsi);
>>}
>> @@ -3405,6 +3409,8 @@ replace_rhs_if_not_dup (enum tree_code new_code, t
>>   gimple_assign_set_rhs_with_ops (, new_code, new_rhs1, new_rhs2);
>>   update_stmt (gsi_stmt (gsi));
>>   c->cand_stmt = gsi_stmt (gsi);
>> +  if (c->next_interp)
>> +   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
>> 
>>   if (dump_file && (dump_flags & TDF_DETAILS))
>>return gsi_stmt (gsi);
>> @@ -3511,6 +3517,8 @@ replace_one_candidate 

Re: [PATCH] Fix PR80158

[Richard Biener]

On Thu, Mar 23, 2017 at 7:20 PM, Bill Schmidt
 wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80158 reports an ICE in
> SLSR while building 416.gamess on x86_64.  This is a latent (but
> previously harmless) bug that was exposed by the fix for PR80054.
> When replacing any statement with a strength reduction, SLSR updates
> the candidate statement S associated with the associated candidate
> record in the candidate table.  However, S may actually be associated
> with two candidate records when an expression may be treated as
> either a CAND_ADD or a CAND_MULT.  In this case, the second one can
> be reached via the next_interp field.

Hmm, the following code in SLSR suggests there may be more than
one such alternate candidate:

  while (arg_cand->kind != CAND_ADD && arg_cand->kind != CAND_PHI)
{
  if (!arg_cand->next_interp)
return;

  arg_cand = lookup_cand (arg_cand->next_interp);
}

> In the excerpt from 416.gamess, the first candidate record was
> updated when its statement was replaced, but the next-interp record
> was not.  Later, that record was used as a basis for another tree
> of strength-reduction candidates, but since it now pointed to an
> orphaned statement without a basic block, the ICE occurred when
> checking dominance against the statement's block.
>
> The fix is simply to ensure that the next_interp record is always
> updated when present.
>
> I've bootstrapped and tested this on powerpc64le-unknown-linux-gnu
> with no regressions.  I tested that the standalone test works on
> x86_64 with this fix, but I have been unable to perform an x86_64
> regstrap because the machine I have access to has insufficient
> disk space. :/  I would appreciate it if you could do this for me.
>
> Assuming no problems with x86_64 regstrap, is this ok for trunk?
>
> Note: I will be unreachable from now until next Tuesday (i.e.,
> returning the 28th).  If needed, please feel free to commit the
> patch on my behalf.  Otherwise I will attend to it when I return.

I'll do a regstrap and gamess build and will commit if that succeeds.

The above can be addressed as followup.

Thanks,
Richard.

> Thanks!
> Bill
>
>
> [gcc]
>
> 2017-03-23  Bill Schmidt  
>
> PR tree-optimization/80158
> * gimple-ssa-strength-reduction.c (replace_mult_candidate): When
> replacing a candidate statement, also replace it for the
> candidate's alternate interpretation.
> (replace_rhs_if_not_dup): Likewise.
> (replace_one_candidate): Likewise.
>
> [gcc/testsuite]
>
> 2017-03-23  Bill Schmidt  
>
> PR tree-optimization/80158
> * gfortran.fortran-torture/compile/pr80158.f: New file.
>
>
> Index: gcc/gimple-ssa-strength-reduction.c
> ===
> --- gcc/gimple-ssa-strength-reduction.c (revision 246420)
> +++ gcc/gimple-ssa-strength-reduction.c (working copy)
> @@ -2089,6 +2089,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
>   gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
>   gsi_replace (, copy_stmt, false);
>   c->cand_stmt = copy_stmt;
> + if (c->next_interp)
> +   lookup_cand (c->next_interp)->cand_stmt = copy_stmt;
>   if (dump_file && (dump_flags & TDF_DETAILS))
> stmt_to_print = copy_stmt;
> }
> @@ -2118,6 +2120,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
>   basis_name, bump_tree);
>   update_stmt (gsi_stmt (gsi));
>c->cand_stmt = gsi_stmt (gsi);
> + if (c->next_interp)
> +   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
>   if (dump_file && (dump_flags & TDF_DETAILS))
> stmt_to_print = gsi_stmt (gsi);
> }
> @@ -3405,6 +3409,8 @@ replace_rhs_if_not_dup (enum tree_code new_code, t
>gimple_assign_set_rhs_with_ops (, new_code, new_rhs1, new_rhs2);
>update_stmt (gsi_stmt (gsi));
>c->cand_stmt = gsi_stmt (gsi);
> +  if (c->next_interp)
> +   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
>
>if (dump_file && (dump_flags & TDF_DETAILS))
> return gsi_stmt (gsi);
> @@ -3511,6 +3517,8 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
>   gimple_assign_set_rhs_with_ops (, MINUS_EXPR, basis_name, rhs2);
>   update_stmt (gsi_stmt (gsi));
>c->cand_stmt = gsi_stmt (gsi);
> + if (c->next_interp)
> +   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
>
>   if (dump_file && (dump_flags & TDF_DETAILS))
> stmt_to_print = gsi_stmt (gsi);
> @@ -3532,6 +3540,8 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
>   gimple_set_location (copy_stmt, gimple_location 

[PATCH] Fix PR80158

[Bill Schmidt]

Hi,

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80158 reports an ICE in
SLSR while building 416.gamess on x86_64.  This is a latent (but
previously harmless) bug that was exposed by the fix for PR80054.
When replacing any statement with a strength reduction, SLSR updates
the candidate statement S associated with the associated candidate
record in the candidate table.  However, S may actually be associated
with two candidate records when an expression may be treated as
either a CAND_ADD or a CAND_MULT.  In this case, the second one can
be reached via the next_interp field.

In the excerpt from 416.gamess, the first candidate record was
updated when its statement was replaced, but the next-interp record
was not.  Later, that record was used as a basis for another tree
of strength-reduction candidates, but since it now pointed to an
orphaned statement without a basic block, the ICE occurred when
checking dominance against the statement's block.

The fix is simply to ensure that the next_interp record is always
updated when present.

I've bootstrapped and tested this on powerpc64le-unknown-linux-gnu
with no regressions.  I tested that the standalone test works on
x86_64 with this fix, but I have been unable to perform an x86_64
regstrap because the machine I have access to has insufficient
disk space. :/  I would appreciate it if you could do this for me.

Assuming no problems with x86_64 regstrap, is this ok for trunk?

Note: I will be unreachable from now until next Tuesday (i.e.,
returning the 28th).  If needed, please feel free to commit the
patch on my behalf.  Otherwise I will attend to it when I return.

Thanks!
Bill


[gcc]

2017-03-23  Bill Schmidt  

PR tree-optimization/80158
* gimple-ssa-strength-reduction.c (replace_mult_candidate): When
replacing a candidate statement, also replace it for the
candidate's alternate interpretation.
(replace_rhs_if_not_dup): Likewise.
(replace_one_candidate): Likewise.

[gcc/testsuite]

2017-03-23  Bill Schmidt  

PR tree-optimization/80158
* gfortran.fortran-torture/compile/pr80158.f: New file.


Index: gcc/gimple-ssa-strength-reduction.c
===
--- gcc/gimple-ssa-strength-reduction.c (revision 246420)
+++ gcc/gimple-ssa-strength-reduction.c (working copy)
@@ -2089,6 +2089,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
  gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
  gsi_replace (, copy_stmt, false);
  c->cand_stmt = copy_stmt;
+ if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = copy_stmt;
  if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = copy_stmt;
}
@@ -2118,6 +2120,8 @@ replace_mult_candidate (slsr_cand_t c, tree basis_
  basis_name, bump_tree);
  update_stmt (gsi_stmt (gsi));
   c->cand_stmt = gsi_stmt (gsi);
+ if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
  if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = gsi_stmt (gsi);
}
@@ -3405,6 +3409,8 @@ replace_rhs_if_not_dup (enum tree_code new_code, t
   gimple_assign_set_rhs_with_ops (, new_code, new_rhs1, new_rhs2);
   update_stmt (gsi_stmt (gsi));
   c->cand_stmt = gsi_stmt (gsi);
+  if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
 
   if (dump_file && (dump_flags & TDF_DETAILS))
return gsi_stmt (gsi);
@@ -3511,6 +3517,8 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
  gimple_assign_set_rhs_with_ops (, MINUS_EXPR, basis_name, rhs2);
  update_stmt (gsi_stmt (gsi));
   c->cand_stmt = gsi_stmt (gsi);
+ if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = gsi_stmt (gsi);
 
  if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = gsi_stmt (gsi);
@@ -3532,6 +3540,8 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
  gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
  gsi_replace (, copy_stmt, false);
  c->cand_stmt = copy_stmt;
+ if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = copy_stmt;
 
  if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = copy_stmt;
@@ -3543,6 +3553,8 @@ replace_one_candidate (slsr_cand_t c, unsigned i,
  gimple_set_location (cast_stmt, gimple_location (c->cand_stmt));
  gsi_replace (, cast_stmt, false);
  c->cand_stmt = cast_stmt;
+ if (c->next_interp)
+   lookup_cand (c->next_interp)->cand_stmt = cast_stmt;
 
  if (dump_file && (dump_flags & TDF_DETAILS))
stmt_to_print = cast_stmt;
Index: