Message ID | 82038b32-b165-f588-7e01-ddcec0a327cb@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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 >> +
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 +