Fix PR80158

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  <wschmidt@linux.vnet.ibm.com>

	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  <wschmidt@linux.vnet.ibm.com>

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

On Thu, Mar 23, 2017 at 7:20 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>
>         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  <wschmidt@linux.vnet.ibm.com>
>
>         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 (&gsi, 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 (&gsi, 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 (&gsi, 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 (&gsi, 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 (&gsi, 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: gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f
> ===================================================================
> --- gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f    (nonexistent)
> +++ gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f    (working copy)
> @@ -0,0 +1,16 @@
> +      SUBROUTINE DRPAUL(SMAT,TMAT,EPS,EPT,SIJ,TIJ,WRK,VEC,ARRAY,FMO,
> +     *                  XMKVIR,TMJ,XMI,YMI,ZMI,ZQQ,L1,L1EF,LNA,LNA2,
> +     *                  NAEF,L2,NLOC,NVIR,PROVEC,FOCKMA,MXBF,MXMO2)
> +      DIMENSION CMO(L1,L1),TLOC(LNA,LNA),SMJ(L1,NAEF),XMK(L1,LNA)
> +      DO I = 1,LNA
> +         DO J = 1,LNA
> +            IF (I.LE.NOUT) TLOC(I,J) = ZERO
> +            IF (J.LE.NOUT) TLOC(I,J) = ZERO
> +         END DO
> +         DO NA=1,NOC
> +            IF ( ABS(E(NI)-E(NA)) .GE.TOL) THEN
> +            END IF
> +         END DO
> +      END DO
> +      END
> +
>

On Mar 24, 2017, at 3:19 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Thu, Mar 23, 2017 at 7:20 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>> 
>>        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  <wschmidt@linux.vnet.ibm.com>
>> 
>>        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 (&gsi, 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 (&gsi, 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 (&gsi, 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 (&gsi, 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 (&gsi, 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: gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f
>> ===================================================================
>> --- gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f    (nonexistent)
>> +++ gcc/testsuite/gfortran.fortran-torture/compile/pr80158.f    (working copy)
>> @@ -0,0 +1,16 @@
>> +      SUBROUTINE DRPAUL(SMAT,TMAT,EPS,EPT,SIJ,TIJ,WRK,VEC,ARRAY,FMO,
>> +     *                  XMKVIR,TMJ,XMI,YMI,ZMI,ZQQ,L1,L1EF,LNA,LNA2,
>> +     *                  NAEF,L2,NLOC,NVIR,PROVEC,FOCKMA,MXBF,MXMO2)
>> +      DIMENSION CMO(L1,L1),TLOC(LNA,LNA),SMJ(L1,NAEF),XMK(L1,LNA)
>> +      DO I = 1,LNA
>> +         DO J = 1,LNA
>> +            IF (I.LE.NOUT) TLOC(I,J) = ZERO
>> +            IF (J.LE.NOUT) TLOC(I,J) = ZERO
>> +         END DO
>> +         DO NA=1,NOC
>> +            IF ( ABS(E(NI)-E(NA)) .GE.TOL) THEN
>> +            END IF
>> +         END DO
>> +      END DO
>> +      END
>> +