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); 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 build_and_add_sum. Why not move the if (stmt1) insert; if (stmt2) insert; before the if () unconditionally? Do we make progress with just the rest of the changes? If so please split the patch and include relevant testcases. Thanks, Richard. > Thanks, > Kugan
