On Fri, May 27, 2016 at 2:36 PM, Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote: > Hi Richard, > > On 27 May 2016 at 19:56, Richard Biener <richard.guent...@gmail.com> wrote: >> On Thu, May 26, 2016 at 11:32 AM, Kugan Vivekanandarajah >> <kugan.vivekanandara...@linaro.org> wrote: >>> Hi Jakub, >>> >>> >>> On 26 May 2016 at 18:18, Jakub Jelinek <ja...@redhat.com> wrote: >>>> On Thu, May 26, 2016 at 02:17:56PM +1000, Kugan Vivekanandarajah wrote: >>>>> --- a/gcc/tree-ssa-reassoc.c >>>>> +++ b/gcc/tree-ssa-reassoc.c >>>>> @@ -3767,8 +3767,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops, >>>>> operand_entry temp = *oe3; >>>>> oe3->op = oe1->op; >>>>> oe3->rank = oe1->rank; >>>>> + oe3->stmt_to_insert = oe1->stmt_to_insert; >>>>> oe1->op = temp.op; >>>>> oe1->rank= temp.rank; >>>>> + oe1->stmt_to_insert = temp.stmt_to_insert; >>>> >>>> If you want to swap those 3 fields (what about the others?), can't you >>>> write >>>> std::swap (oe1->op, oe3->op); >>>> std::swap (oe1->rank, oe3->rank); >>>> std::swap (oe1->stmt_to_insert, oe3->stmt_to_insert); >>>> instead and drop operand_entry temp = *oe3; ? >>>> >>>>> } >>>>> else if ((oe1->rank == oe3->rank >>>>> && oe2->rank != oe3->rank) >>>>> @@ -3779,8 +3781,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops, >>>>> operand_entry temp = *oe2; >>>>> oe2->op = oe1->op; >>>>> oe2->rank = oe1->rank; >>>>> + oe2->stmt_to_insert = oe1->stmt_to_insert; >>>>> oe1->op = temp.op; >>>>> oe1->rank = temp.rank; >>>>> + oe1->stmt_to_insert = temp.stmt_to_insert; >>>>> } >>>> >>>> Similarly. >>> >>> Done. Revised patch attached. >> >> Your patch only adds a single testcase, please make sure to include >> _all_ relevant testcases. >> >> The swap should simply swap the whole operand, thus >> >> std::swap (*oe1, *oe3); >> > > Thanks for the review. > > I will change this. > >> it was probably not updated when all the other fields were added. >> >> I don't like the find_insert_point changes or the change before > > If we insert the stmt_to_insert before the find_insert_point, we can > end up inserting stmt_to_insert before its argument defining stmt. > This can be seen with f951 cp2k_single_file.f90 -O3 -ffast-math > -march=westmere from PR71252. I am attaching the CFG when all the > insert_stmt_before_use are moved before.
Hmm, but then this effectively means we should have find_insert_point for inserting to_insert stmts in the first place? That is, in insert_stmt_before_use use find_insert_point on the stmt_to_insert ops? > I dont understand Fortran and I am not able to reduce a testcase from this. > >> build_and_add_sum. >> Why not move the if (stmt1) insert; if (stmt2) insert; before the if >> () unconditionally? > > In this case also, we dont know where build_and_add_sum will insert > the new instruction. It may not be stmts[i] before calling > build_and_add_sum. Therefore we can end up inserting in a wrong place. > testcase gfortran.dg/pr71252.f90 would ICE. So split off build_and_add_sum_1 which does not do stmt insertion and instead treat it like a to_insert stmt at this point (simply insert it at stmts[i] or using find_insert_point)? >> >> Do we make progress with just the rest of the changes? If so please split >> the >> patch and include relevant testcases. > > I think there are two issues. > 1. the swap which is obvious > 2. insertion poing which has some related changes and shows two > different problems (listed above) > > if you prefer, I can send two patches for the above. Yes please. > Unfortunately, I am not able to reduce Fortran testcase. Any help from > anyone is really appreciated. > > > Thanks, > Kugan > >> >> Thanks, >> Richard. >> >>> Thanks, >>> Kugan
