On Fri, Sep 6, 2013 at 8:47 PM, Easwaran Raman <era...@google.com> wrote: > On Fri, Sep 6, 2013 at 12:05 AM, Richard Biener > <richard.guent...@gmail.com> wrote: >> On Thu, Sep 5, 2013 at 9:23 PM, Easwaran Raman <era...@google.com> wrote: >>> On Tue, Aug 27, 2013 at 3:35 AM, Richard Biener >>> <richard.guent...@gmail.com> wrote: >>>> On Thu, Aug 1, 2013 at 1:34 AM, Easwaran Raman <era...@google.com> wrote: >>>>> I have a new patch that supersedes this. The new patch also fixes PR >>>>> tree-optimization/57393 and PR tree-optimization/58011. Bootstraps and >>>>> no test regression on x86_64/linux. Ok for trunk? >>>>> >>>>> 2013-07-31 Easwaran Raman <era...@google.com> >>>>> >>>>> PR middle-end/57370 >>>>> * tree-ssa-reassoc.c (build_and_add_sum): Fix UID assignment and reset >>>>> of debug statements that cause inconsistent IR. >>>> >>>> Missing ChangeLog entry for the insert_stmt_after hunk which I do not like >>>> at all. The other hunks are ok, but we need to work harder to preserve >>>> debug stmts - simply removing all is not going to fly. >>>> >>>> Richard. >>> >>> I looked into the problem related to the debug stmts in this failing >>> test case in detail. Initially, the code sequence looks >>> >>> >>> s$n_13 = MEM[(struct S *)&s]; >>> # DEBUG s$n => s$n_13 >>> _2 = (double) s$n_13; >>> _4 = _2 * a_3(D); >>> _6 = (double) i_5(D); >>> _7 = _4 * _6; >>> _9 = (double) j_8(D); >>> _10 = _7 * _9; >>> bar (_10); >>> # DEBUG D#2 => (double) k_12(D) >>> # DEBUG D#1 => _7 * D#2 >>> # DEBUG t$n => (int) D#1 >>> >>> After reassociation >>> >>> s$n_13 = MEM[(struct S *)&s]; >>> # DEBUG s$n => s$n_13 >>> _2 = (double) s$n_13; >>> _6 = (double) i_5(D); >>> # DEBUG D#3 => _4 * _6 >>> _9 = (double) j_8(D); >>> _4 = _9 * _2; >>> _7 = _4 * a_3(D); >>> _10 = _7 * _6; >>> bar (_10); >>> # DEBUG D#2 => (double) k_12(D) >>> # DEBUG D#1 => D#3 * D#2 >>> # DEBUG t$n => (int) D#1 >>> >>> In the above, # DEBUG D#3 => _4 * _6 appears above the statement that >>> defines _4. But even if I move the def of D#3 below the statement >>> defining _4, it is not sufficient. Before reassociation, t$n refers to >>> the expression (s$n_13 * a_3(D) * i_5(D) * k_12(D)), but after >>> reassociation it would refer to ( j_8(D) * s$n_13 * i_5(D) * >>> k_12(D)). It seems the correct fix is to discard the debug temps whose >>> RHS contains a variable that is involved in reassociation and then >>> reconstruct it. Is that the expected fix here? >> >> The value of the DEBUG expression changes because the value >> that _4 computes changes - that is a no-no that may not happen. >> We cannot re-use _4 (without previously releasing it and allocating >> it newly) with a new value. releasing _4 should have fixed up the >> debug stmts. >> >> So - can you verify whether we are indeed just replacing the >> RHS of _4 = _2 * a_3(D) to _9 * _2 without changing the SSA name of >> that expression? > > I have confirmed that the SSA name of the LHS remains the same. The > reassociation code simply calls gimple_assign_set_rhs2 to modify RHS2 > and then calls update_stmt.
That's definitely not allowed (though ISTR I remember that reassoc does this kind of things :/) - it will result in wrong debug info. Richard. > - Easwaran > >> Another reason why re-using the same LHS for another value is >> wrong is that annotated information like points-to sets, alignment >> and soon value-range information will be wrong. >> >> Thanks, >> Richard. >> >>> - Easwaran >>> >>>>> >>>>> testsuite/ChangeLog: >>>>> 2013-07-31 Easwaran Raman <era...@google.com> >>>>> >>>>> PR middle-end/57370 >>>>> PR tree-optimization/57393 >>>>> PR tree-optimization/58011 >>>>> * gfortran.dg/reassoc_12.f90: New testcase. >>>>> * gcc.dg/tree-ssa/reassoc-31.c: New testcase. >>>>> * gcc.dg/tree-ssa/reassoc-31.c: New testcase. >>>>> >>>>> >>>>> On Fri, Jul 12, 2013 at 7:57 AM, Easwaran Raman <era...@google.com> wrote: >>>>>> Ping. >>>>>> >>>>>> On Thu, Jun 27, 2013 at 10:15 AM, Easwaran Raman <era...@google.com> >>>>>> wrote: >>>>>>> A newly generated statement in build_and_add_sum function of >>>>>>> tree-ssa-reassoc.c has to be assigned the UID of its adjacent >>>>>>> statement. In one instance, it was assigned the wrong uid (of an >>>>>>> earlier phi statement) which messed up the IR and caused the test >>>>>>> program to hang. Bootstraps and no test regressions on x86_64/linux. >>>>>>> Ok for trunk? >>>>>>> >>>>>>> Thanks, >>>>>>> Easwaran >>>>>>> >>>>>>> >>>>>>> 2013-06-27 Easwaran Raman <era...@google.com> >>>>>>> >>>>>>> PR middle-end/57370 >>>>>>> * tree-ssa-reassoc.c (build_and_add_sum): Do not use the UID of >>>>>>> a phi >>>>>>> node for a non-phi gimple statement. >>>>>>> >>>>>>> testsuite/ChangeLog: >>>>>>> 2013-06-27 Easwaran Raman <era...@google.com> >>>>>>> >>>>>>> PR middle-end/57370 >>>>>>> * gfortran.dg/reassoc_12.f90: New testcase. >>>>>>> >>>>>>> >>>>>>> Index: gcc/testsuite/gfortran.dg/reassoc_12.f90 >>>>>>> =================================================================== >>>>>>> --- gcc/testsuite/gfortran.dg/reassoc_12.f90 (revision 0) >>>>>>> +++ gcc/testsuite/gfortran.dg/reassoc_12.f90 (revision 0) >>>>>>> @@ -0,0 +1,74 @@ >>>>>>> +! { dg-do compile } >>>>>>> +! { dg-options "-O2 -ffast-math" } >>>>>>> +! PR middle-end/57370 >>>>>>> + >>>>>>> + SUBROUTINE xb88_lr_adiabatic_lda_calc(e_ndrho_ndrho_ndrho, & >>>>>>> + grad_deriv,npoints, sx) >>>>>>> + IMPLICIT REAL*8 (t) >>>>>>> + INTEGER, PARAMETER :: dp=8 >>>>>>> + REAL(kind=dp), DIMENSION(1:npoints) :: e_ndrho_ndrho_ndrho, & >>>>>>> + e_ndrho_ndrho_rho >>>>>>> + DO ii=1,npoints >>>>>>> + IF( grad_deriv >= 2 .OR. grad_deriv == -2 ) THEN >>>>>>> + t1425 = t233 * t557 >>>>>>> + t1429 = beta * t225 >>>>>>> + t1622 = t327 * t1621 >>>>>>> + t1626 = t327 * t1625 >>>>>>> + t1632 = t327 * t1631 >>>>>>> + t1685 = t105 * t1684 >>>>>>> + t2057 = t1636 + t8 * (t2635 + t3288) >>>>>>> + END IF >>>>>>> + IF( grad_deriv >= 3 .OR. grad_deriv == -3 ) THEN >>>>>>> + t5469 = t5440 - t5443 - t5446 - t5449 - & >>>>>>> + t5451 - t5454 - t5456 + t5459 - & >>>>>>> + t5462 + t5466 - t5468 >>>>>>> + t5478 = 0.240e2_dp * t1616 * t973 * t645 * t1425 >>>>>>> + t5489 = 0.1600000000e2_dp * t1429 * t1658 >>>>>>> + t5531 = 0.160e2_dp * t112 * t1626 >>>>>>> + t5533 = 0.160e2_dp * t112 * t1632 >>>>>>> + t5537 = 0.160e2_dp * t112 * t1622 >>>>>>> + t5541 = t5472 - t5478 - t5523 + t5525 + & >>>>>>> + t5531 + t5533 + t5535 + t5537 + & >>>>>>> + t5540 >>>>>>> + t5565 = t112 * t1685 >>>>>>> + t5575 = t5545 - t5548 + t5551 + t5553 - & >>>>>>> + t5558 + t5560 - t5562 + t5564 - & >>>>>>> + 0.80e1_dp * t5565 + t5568 + t5572 + & >>>>>>> + t5574 >>>>>>> + t5611 = t5579 - t5585 + t5590 - t5595 + & >>>>>>> + t5597 - t5602 + t5604 + t5607 + & >>>>>>> + t5610 >>>>>>> + t5613 = t5469 + t5541 + t5575 + t5611 >>>>>>> + t6223 = t6189 - & >>>>>>> + 0.3333333336e0_dp * t83 * t84 * t5613 + & >>>>>>> + t6222 >>>>>>> + t6227 = - t8 * (t5305 + t6223) >>>>>>> + e_ndrho_ndrho_rho(ii) = e_ndrho_ndrho_rho(ii) + & >>>>>>> + t6227 * sx >>>>>>> + t6352 = t5440 - t5443 - t5446 - t5449 - & >>>>>>> + t5451 - t5454 + & >>>>>>> + 0.40e1_dp * t102 * t327 * t2057 * t557 - & >>>>>>> + t5456 + t5459 - t5462 + t5466 - & >>>>>>> + t5468 >>>>>>> + t6363 = t5480 - t5489 + & >>>>>>> + 0.9600000000e2_dp * t1054 * t640 * t3679 >>>>>>> + t6367 = t5472 - t5474 - t5478 - t5523 + & >>>>>>> + t5525 + t5531 + t5533 + t5535 + & >>>>>>> + t5537 - 0.20e1_dp * t102 * t105 * t6363 + & >>>>>>> + t5540 >>>>>>> + t6370 = t5545 - t5548 + t5551 + t5553 - & >>>>>>> + t5558 + t5560 - t5562 + t5564 - & >>>>>>> + 0.40e1_dp * t5565 + & >>>>>>> + t5568 + t5572 + t5574 >>>>>>> + t6373 = t5579 - t5585 + t5590 - t5595 + & >>>>>>> + t5597 - t5602 + t5604 + t5607 + & >>>>>>> + t5610 >>>>>>> + t6375 = t6352 + t6367 + t6370 + t6373 >>>>>>> + t6380 = - 0.3333333336e0_dp * t83 * t84 * t6375 + t5701 >>>>>>> + t6669 = -t4704 - t8 * (t6344 + t6380 + t6665) >>>>>>> + e_ndrho_ndrho_ndrho(ii) = e_ndrho_ndrho_ndrho(ii) + & >>>>>>> + t6669 * sx >>>>>>> + END IF >>>>>>> + END DO >>>>>>> + END SUBROUTINE xb88_lr_adiabatic_lda_calc >>>>>>> + >>>>>>> Index: gcc/tree-ssa-reassoc.c >>>>>>> =================================================================== >>>>>>> --- gcc/tree-ssa-reassoc.c (revision 200429) >>>>>>> +++ gcc/tree-ssa-reassoc.c (working copy) >>>>>>> @@ -1207,7 +1207,7 @@ build_and_add_sum (tree type, tree op1, tree op2, >>>>>>> if (gimple_code (op1def) == GIMPLE_PHI) >>>>>>> { >>>>>>> gsi = gsi_after_labels (gimple_bb (op1def)); >>>>>>> - gimple_set_uid (sum, gimple_uid (op1def)); >>>>>>> + gimple_set_uid (sum, gimple_uid (gsi_stmt (gsi))); >>>>>>> gsi_insert_before (&gsi, sum, GSI_NEW_STMT); >>>>>>> } >>>>>>> else