Re: [PATCH PR69652, Regression]

[Yuri Rumyantsev <ysrumyan at gmail dot com>]

Hi All,

Here is updated patch - I came back to move call statements also since
 masked loads are presented by internal call. I also assume that for
the following simple loop
  for (i = 0; i < n; i++)
    if (b1[i])
      a1[i] = sqrtf(a2[i] * a2[i] + a3[i] * a3[i]);
motion must be done for all vector statements in semi-hammock including SQRT.

Bootstrap and regression testing did not show any new failures.
Is it OK for trunk?

ChangeLog:

2016-02-05  Yuri Rumyantsev  <ysrumyan@gmail.com>

PR tree-optimization/69652
* tree-vect-loop.c (optimize_mask_stores): Move declaration of STMT1
to nested loop, introduce new SCALAR_VUSE vector to keep vuse of all
skipped scalar statements, introduce variable LAST_VUSE to keep
vuse of LAST_STORE, add assertion that SCALAR_VUSE is empty in the
begining of current masked store processing, did source re-formatting,
skip parsing of debug gimples, stop processing if a gimple with
volatile operand has been encountered, save scalar statement
with vuse in SCALAR_VUSE, skip processing debug statements in IMM_USE
iterator, change vuse of all saved scalar statements to LAST_VUSE if
it makes sence.

gcc/testsuite/ChangeLog:
* gcc.dg/torture/pr69652.c: New test.

2016-02-04 19:40 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> On Thu, Feb 04, 2016 at 05:46:27PM +0300, Yuri Rumyantsev wrote:
>> Here is a patch that cures the issues with non-correct vuse for scalar
>> statements during code motion, i.e. if vuse of scalar statement is
>> vdef of masked store which has been sunk to new basic block, we must
>> fix it up.  The patch also fixed almost all remarks pointed out by
>> Jacub.
>>
>> Bootstrapping and regression testing on v86-64 did not show any new failures.
>> Is it OK for trunk?
>>
>> ChangeLog:
>> 2016-02-04  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR tree-optimization/69652
>> * tree-vect-loop.c (optimize_mask_stores): Move declaration of STMT1
>> to nested loop, introduce new SCALAR_VUSE vector to keep vuse of all
>> skipped scalar statements, introduce variable LAST_VUSE that has
>> vuse of LAST_STORE, add assertion that SCALAR_VUSE is empty in the
>> begining of current masked store processing, did source re-formatting,
>> skip parsing of debug gimples, stop processing when call or gimple
>> with volatile operand habe been encountered, save scalar statement
>> with vuse in SCALAR_VUSE, skip processing debug statements in IMM_USE
>> iterator, change vuse of all saved scalar statements to LAST_VUSE if
>> it makes sence.
>>
>> gcc/testsuite/ChangeLog:
>> * gcc.dg/torture/pr69652.c: New test.
>
> Your mailer breaks ChangeLog formatting, so it is hard to check the
> formatting of the ChangeLog entry.
>
> diff --git a/gcc/testsuite/gcc.dg/torture/pr69652.c b/gcc/testsuite/gcc.dg/torture/pr69652.c
> new file mode 100644
> index 0000000..91f30cf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr69652.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -ffast-math -ftree-vectorize " } */
> +/* { dg-additional-options "-mavx" { target { i?86-*-* x86_64-*-* } } } */
> +
> +void fn1(double **matrix, int column, int row, int n)
> +{
> +  int k;
> +  for (k = 0; k < n; k++)
> +    if (matrix[row][k] != matrix[column][k])
> +      {
> +       matrix[column][k] = -matrix[column][k];
> +       matrix[row][k] = matrix[row][k] - matrix[column][k];
> +      }
> +}
> \ No newline at end of file
>
> Please make sure the last line of the test is a new-line.
>
> @@ -6971,6 +6972,8 @@ optimize_mask_stores (struct loop *loop)
>            gsi_next (&gsi))
>         {
>           stmt = gsi_stmt (gsi);
> +         if (is_gimple_debug (stmt))
> +           continue;
>           if (is_gimple_call (stmt)
>               && gimple_call_internal_p (stmt)
>               && gimple_call_internal_fn (stmt) == IFN_MASK_STORE)
>
> This is not needed, you do something only for is_gimple_call,
> which is never true if is_gimple_debug, so the code used to be fine as is.
>
> +             /* Skip debug sstatements.  */
>
> s/ss/s/
>
> +             if (is_gimple_debug (gsi_stmt (gsi)))
> +               continue;
> +             stmt1 = gsi_stmt (gsi);
> +             /* Do not consider writing to memory,volatile and call
>
> Missing space after ,
>
> +             /* Skip scalar statements.  */
> +             if (!VECTOR_TYPE_P (TREE_TYPE (lhs)))
> +               {
> +                 /* If scalar statement has vuse we need to modify it
> +                    when another masked store will be sunk.  */
> +                 if (gimple_vuse (stmt1))
> +                   scalar_vuse.safe_push (stmt1);
>                   continue;
> +               }
>
> I don't think it is safe to take for granted that the scalar stmts are all
> going to be DCEd, but I could be wrong.
>
> +             /* Check that LHS does not have uses outside of STORE_BB.  */
> +             res = true;
> +             FOR_EACH_IMM_USE_FAST (use_p, imm_iter, lhs)
> +               {
> +                 gimple *use_stmt;
> +                 use_stmt = USE_STMT (use_p);
> +                 if (is_gimple_debug (use_stmt))
> +                   continue;
>
> Ignoring debug stmts to make decision whether you move or not is
> of course the right thing to do.  But IMHO you should remember if
> you saw any is_gimple_debug stmts in some bool var.
>
> +                 if (gimple_bb (use_stmt) != store_bb)
> +                   {
> +                     res = false;
> +                     break;
> +                   }
> +               }
> +             if (!res)
> +               break;
>
> -               if (gimple_vuse (stmt1)
> -                   && gimple_vuse (stmt1) != gimple_vuse (last_store))
> -                 break;
> +             if (gimple_vuse (stmt1)
> +                 && gimple_vuse (stmt1) != gimple_vuse (last_store))
> +               break;
>
> +             /* Can move STMT1 to STORE_BB.  */
> +             if (dump_enabled_p ())
> +               {
> +                 dump_printf_loc (MSG_NOTE, vect_location,
> +                                  "Move stmt to created bb\n");
> +                 dump_gimple_stmt (MSG_NOTE, TDF_SLIM, stmt1, 0);
> +               }
>
> And if yes, invalidate them here, because the move would otherwise
> generate invalid IL.
>
> +             gsi_move_before (&gsi_from, &gsi_to);
> +             /* Shift GSI_TO for further insertion.  */
> +             gsi_prev (&gsi_to);
> +           }
> +         /* Put other masked stores with the same mask to STORE_BB.  */
> +         if (worklist.is_empty ()
> +             || gimple_call_arg (worklist.last (), 2) != mask
> +             || worklist.last () != stmt1)
> +           break;
> +         last = worklist.pop ();
>         }
>        add_phi_arg (phi, gimple_vuse (last_store), e, UNKNOWN_LOCATION);
> +      /* Mask stores motion could crossing scalar statements with vuse
> +        which should be corrected.  */
>
> s/crossing/cross/
> That said, I'm not really sure if without some verification if such
> reads are really dead it is safe to skip them and update this way.
> Richard?
>
> +      last_vuse = gimple_vuse (last_store);
> +      while (!scalar_vuse.is_empty ())
> +       {
> +          stmt = scalar_vuse.pop ();
> +          if (gimple_vuse (stmt) != last_vuse)
> +             {
> +               gimple_set_vuse (stmt, last_vuse);
> +               update_stmt (stmt);
> +             }
> +       }
>      }
>  }
>
>         Jakub

Attachment: patch.1
Description: Binary data