On Fri, Feb 5, 2016 at 3:54 PM, Yuri Rumyantsev <ysrum...@gmail.com> wrote:
> 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?
The patch is incredibly hard to parse due to the re-indenting. Please
consider sending
diffs with -b.
This issue exposes that you are moving (masked) stores across loads without
checking aliasing. In the specific case those loads are dead and thus
this is safe
but in general I thought we were checking that we are using the same VUSE
during the sinking operation.
Thus, I'd rather have
+ /* 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;
+ if (gimple_bb (use_stmt) != store_bb)
+ {
+ res = false;
+ break;
+ }
+ }
also check for the dead code case and DCE those stmts here. Like so:
if (has_zero_uses (lhs))
{
gsi_remove (&gsi_from, true);
continue;
}
before the above loop.
Richard.
> ChangeLog:
>
> 2016-02-05 Yuri Rumyantsev <ysrum...@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 <ja...@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 <ysrum...@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
