On March 22, 2017 7:52:40 PM GMT+01:00, Jakub Jelinek <ja...@redhat.com> wrote:
>Hi!
>
>On the huge testcase from the PR (not really reduceable) we run into
>reassoc ranks bigger than INT_MAX (bb_rank is multiples of 65536,
>so it might be enough to have just 32768+ basic blocks to reach that).
>Rank is unsigned int, but we were returning (int) (oeb->rank -
>oea->rank);
>from the comparison function, which means when unlucky enough it could
>sort
>entries with huge rank e.g. after entries with 0 rank. Rest of reassoc
>relies on the constants (rank 0) being sorted last though.
>
>The patch also applies similar changes to other fields, that are far
>less
>likely to be huge, but could be in theory.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
>2017-03-22 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/80072
> * tree-ssa-reassoc.c (struct operand_entry): Change id field type
> to unsigned int.
> (next_operand_entry_id): Change type to unsigned int.
> (sort_by_operand_rank): Make sure to return the right return value
> even if unsigned fields are bigger than INT_MAX.
> (struct oecount): Change cnt and id type to unsigned int.
> (oecount_hasher::equal): Formatting fix.
> (oecount_cmp): Make sure to return the right return value
> even if unsigned fields are bigger than INT_MAX.
> (undistribute_ops_list): Change next_oecount_id type to unsigned int.
>
>--- gcc/tree-ssa-reassoc.c.jj 2017-02-10 09:46:50.000000000 +0100
>+++ gcc/tree-ssa-reassoc.c 2017-03-22 13:57:00.476928949 +0100
>@@ -193,7 +193,7 @@ static struct
> struct operand_entry
> {
> unsigned int rank;
>- int id;
>+ unsigned int id;
> tree op;
> unsigned int count;
> gimple *stmt_to_insert;
>@@ -204,7 +204,7 @@ static object_allocator<operand_entry> o
>
> /* This is used to assign a unique ID to each struct operand_entry
> so that qsort results are identical on different hosts. */
>-static int next_operand_entry_id;
>+static unsigned int next_operand_entry_id;
>
> /* Starting rank number for a given basic block, so that we can rank
> operations using unmovable instructions in that BB based on the bb
>@@ -505,12 +505,12 @@ sort_by_operand_rank (const void *pa, co
> else
> /* To make sorting result stable, we use unique IDs to determine
> order. */
>- return oeb->id - oea->id;
>+ return oeb->id > oea->id ? 1 : -1;
> }
>
> /* Lastly, make sure the versions that are the same go next to each
> other. */
>- if ((oeb->rank - oea->rank == 0)
>+ if (oeb->rank == oea->rank
> && TREE_CODE (oea->op) == SSA_NAME
> && TREE_CODE (oeb->op) == SSA_NAME)
> {
>@@ -543,15 +543,15 @@ sort_by_operand_rank (const void *pa, co
> }
>
> if (SSA_NAME_VERSION (oeb->op) != SSA_NAME_VERSION (oea->op))
>- return SSA_NAME_VERSION (oeb->op) - SSA_NAME_VERSION (oea->op);
>+ return SSA_NAME_VERSION (oeb->op) > SSA_NAME_VERSION (oea->op) ? 1 :
>-1;
> else
>- return oeb->id - oea->id;
>+ return oeb->id > oea->id ? 1 : -1;
> }
>
> if (oeb->rank != oea->rank)
>- return oeb->rank - oea->rank;
>+ return oeb->rank > oea->rank ? 1 : -1;
> else
>- return oeb->id - oea->id;
>+ return oeb->id > oea->id ? 1 : -1;
> }
>
> /* Add an operand entry to *OPS for the tree operand OP. */
>@@ -1055,8 +1055,8 @@ static void linearize_expr_tree (vec<ope
>
> /* Structure for tracking and counting operands. */
> struct oecount {
>- int cnt;
>- int id;
>+ unsigned int cnt;
>+ unsigned int id;
> enum tree_code oecode;
> tree op;
> };
>@@ -1090,8 +1090,7 @@ oecount_hasher::equal (int p1, int p2)
> {
> const oecount *c1 = &cvec[p1 - 42];
> const oecount *c2 = &cvec[p2 - 42];
>- return (c1->oecode == c2->oecode
>- && c1->op == c2->op);
>+ return c1->oecode == c2->oecode && c1->op == c2->op;
> }
>
>/* Comparison function for qsort sorting oecount elements by count. */
>@@ -1102,10 +1101,10 @@ oecount_cmp (const void *p1, const void
> const oecount *c1 = (const oecount *)p1;
> const oecount *c2 = (const oecount *)p2;
> if (c1->cnt != c2->cnt)
>- return c1->cnt - c2->cnt;
>+ return c1->cnt > c2->cnt ? 1 : -1;
> else
> /* If counts are identical, use unique IDs to stabilize qsort. */
>- return c1->id - c2->id;
>+ return c1->id > c2->id ? 1 : -1;
> }
>
> /* Return TRUE iff STMT represents a builtin call that raises OP
>@@ -1559,7 +1558,7 @@ undistribute_ops_list (enum tree_code op
> sbitmap_iterator sbi0;
> vec<operand_entry *> *subops;
> bool changed = false;
>- int next_oecount_id = 0;
>+ unsigned int next_oecount_id = 0;
>
> if (length <= 1
> || opcode != PLUS_EXPR)
>
> Jakub
