diff mbox

[PR68542]

Message ID CAFiYyc2Vy7+_9epN8POs7ZU9A3CYYmtpMwZ-2C+F6d=nn_Kx4w@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Jan. 18, 2016, 12:44 p.m. UTC
On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Hi Richard,
>
> Did you have anu chance to look at updated patch?


Ok with that change.

Thanks,
Richard.

> Thanks.
> Yuri.
>
> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>> Hi Richard,
>>
>> Here is updated patch for middle-end part of the whole patch which
>> fixes all your remarks I hope.
>>
>> Regression testing and bootstrapping did not show any new failures.
>> Is it OK for trunk?
>>
>> Yuri.
>>
>> ChangeLog:
>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR middle-end/68542
>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>> of mixind vector and scalar types.
>> (fold_relational_const): Add handling of vector
>> comparison with boolean result.
>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>> comparison of vector operands with boolean result for EQ/NE only.
>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>> (verify_gimple_cond): Likewise.
>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform

Comments

Yuri Rumyantsev Jan. 18, 2016, 2:02 p.m. UTC | #1
Thanks Richard.

I changed the check on type as you proposed.

What about the second back-end part of patch (it has been sent 08.12.15).

Thanks.
Yuri.

2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Hi Richard,
>>
>> Did you have anu chance to look at updated patch?
>
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index acbb70b..208a752 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
> gimple_stmt_iterator si,
>                                                 &comp_code, &val))
>      return;
>
> +  /* VRP doesn't track ranges for vector types.  */
> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
> +    return;
> +
>
> please instead fix extract_code_and_val_from_cond_with_ops with
>
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c      (revision 232506)
> +++ gcc/tree-vrp.c      (working copy)
> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>    if (invert)
>      comp_code = invert_tree_comparison (comp_code, 0);
>
> -  /* VRP does not handle float types.  */
> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
> +  /* VRP only handles integral and pointer types.  */
> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>      return false;
>
>    /* Do not register always-false predicates.
>
> Ok with that change.
>
> Thanks,
> Richard.
>
>> Thanks.
>> Yuri.
>>
>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>> Hi Richard,
>>>
>>> Here is updated patch for middle-end part of the whole patch which
>>> fixes all your remarks I hope.
>>>
>>> Regression testing and bootstrapping did not show any new failures.
>>> Is it OK for trunk?
>>>
>>> Yuri.
>>>
>>> ChangeLog:
>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>
>>> PR middle-end/68542
>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>> of mixind vector and scalar types.
>>> (fold_relational_const): Add handling of vector
>>> comparison with boolean result.
>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>> comparison of vector operands with boolean result for EQ/NE only.
>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>> (verify_gimple_cond): Likewise.
>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Richard Biener Jan. 18, 2016, 2:07 p.m. UTC | #2
On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Thanks Richard.
>
> I changed the check on type as you proposed.
>
> What about the second back-end part of patch (it has been sent 08.12.15).

Can't see it in my inbox - can you reply to the mail with a ping?

Thanks,
Richard.

> Thanks.
> Yuri.
>
> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>> Hi Richard,
>>>
>>> Did you have anu chance to look at updated patch?
>>
>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>> index acbb70b..208a752 100644
>> --- a/gcc/tree-vrp.c
>> +++ b/gcc/tree-vrp.c
>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>> gimple_stmt_iterator si,
>>                                                 &comp_code, &val))
>>      return;
>>
>> +  /* VRP doesn't track ranges for vector types.  */
>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>> +    return;
>> +
>>
>> please instead fix extract_code_and_val_from_cond_with_ops with
>>
>> Index: gcc/tree-vrp.c
>> ===================================================================
>> --- gcc/tree-vrp.c      (revision 232506)
>> +++ gcc/tree-vrp.c      (working copy)
>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>    if (invert)
>>      comp_code = invert_tree_comparison (comp_code, 0);
>>
>> -  /* VRP does not handle float types.  */
>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>> +  /* VRP only handles integral and pointer types.  */
>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>      return false;
>>
>>    /* Do not register always-false predicates.
>>
>> Ok with that change.
>>
>> Thanks,
>> Richard.
>>
>>> Thanks.
>>> Yuri.
>>>
>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>> Hi Richard,
>>>>
>>>> Here is updated patch for middle-end part of the whole patch which
>>>> fixes all your remarks I hope.
>>>>
>>>> Regression testing and bootstrapping did not show any new failures.
>>>> Is it OK for trunk?
>>>>
>>>> Yuri.
>>>>
>>>> ChangeLog:
>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>
>>>> PR middle-end/68542
>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>> of mixind vector and scalar types.
>>>> (fold_relational_const): Add handling of vector
>>>> comparison with boolean result.
>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>> (verify_gimple_cond): Likewise.
>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Yuri Rumyantsev Jan. 18, 2016, 2:50 p.m. UTC | #3
Richard,

Here is the second part of patch which really preforms mask stores and
all statements related to it to new basic block guarded by test on
zero mask. Hew test is also added.

Is it OK for trunk?

Thanks.
Yuri.

2016-01-18  Yuri Rumyantsev  <ysrumyan@gmail.com>

PR middle-end/68542
* config/i386/i386.c (ix86_expand_branch): Implement integral vector
comparison with boolean result.
* config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
for vector comparion with eq/ne only.
* tree-vect-loop.c (is_valid_sink): New function.
(optimize_mask_stores): Likewise.
* tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
has_mask_store field of vect_info.
* tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
vectorized loops having masked stores.
* tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
correspondent macros.
(optimize_mask_stores): Add prototype.

gcc/testsuite/ChangeLog:
* gcc.dg/vect/vect-mask-store-move-1.c: New test.

2016-01-18 17:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Thanks Richard.
>>
>> I changed the check on type as you proposed.
>>
>> What about the second back-end part of patch (it has been sent 08.12.15).
>
> Can't see it in my inbox - can you reply to the mail with a ping?
>
> Thanks,
> Richard.
>
>> Thanks.
>> Yuri.
>>
>> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>> Hi Richard,
>>>>
>>>> Did you have anu chance to look at updated patch?
>>>
>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>> index acbb70b..208a752 100644
>>> --- a/gcc/tree-vrp.c
>>> +++ b/gcc/tree-vrp.c
>>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>>> gimple_stmt_iterator si,
>>>                                                 &comp_code, &val))
>>>      return;
>>>
>>> +  /* VRP doesn't track ranges for vector types.  */
>>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>>> +    return;
>>> +
>>>
>>> please instead fix extract_code_and_val_from_cond_with_ops with
>>>
>>> Index: gcc/tree-vrp.c
>>> ===================================================================
>>> --- gcc/tree-vrp.c      (revision 232506)
>>> +++ gcc/tree-vrp.c      (working copy)
>>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>>    if (invert)
>>>      comp_code = invert_tree_comparison (comp_code, 0);
>>>
>>> -  /* VRP does not handle float types.  */
>>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>>> +  /* VRP only handles integral and pointer types.  */
>>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>>      return false;
>>>
>>>    /* Do not register always-false predicates.
>>>
>>> Ok with that change.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks.
>>>> Yuri.
>>>>
>>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>>> Hi Richard,
>>>>>
>>>>> Here is updated patch for middle-end part of the whole patch which
>>>>> fixes all your remarks I hope.
>>>>>
>>>>> Regression testing and bootstrapping did not show any new failures.
>>>>> Is it OK for trunk?
>>>>>
>>>>> Yuri.
>>>>>
>>>>> ChangeLog:
>>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>
>>>>> PR middle-end/68542
>>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>>> of mixind vector and scalar types.
>>>>> (fold_relational_const): Add handling of vector
>>>>> comparison with boolean result.
>>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>>> (verify_gimple_cond): Likewise.
>>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Richard Biener Jan. 20, 2016, 12:24 p.m. UTC | #4
On Mon, Jan 18, 2016 at 3:50 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Richard,
>
> Here is the second part of patch which really preforms mask stores and
> all statements related to it to new basic block guarded by test on
> zero mask. Hew test is also added.
>
> Is it OK for trunk?

+  /* Pick up all masked stores in loop if any.  */
+  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+    {
+      stmt = gsi_stmt (gsi);

you fail to iterate over all BBs of the loop here.  Please follow
other uses in the
vectorizer.

+  while (!worklist.is_empty ())
+    {
+      gimple *last, *last_store;
+      edge e, efalse;
+      tree mask;
+      basic_block store_bb, join_bb;
+      gimple_stmt_iterator gsi_to;
+      /* tree arg3; */

remove

+      tree vdef, new_vdef;
+      gphi *phi;
+      bool first_dump;
+      tree vectype;
+      tree zero;
+
+      last = worklist.pop ();
+      mask = gimple_call_arg (last, 2);
+      /* Create new bb.  */

bb should be initialized from gimple_bb (last), not loop->header

+      e = split_block (bb, last);

+               gsi_from = gsi_for_stmt (stmt1);
+               gsi_to = gsi_start_bb (store_bb);
+               gsi_move_before (&gsi_from, &gsi_to);
+               update_stmt (stmt1);

I think the update_stmt is redundant and you should be able to
keep two gsis throughout the loop, from and to, no?

+           /* Put other masked stores with the same mask to STORE_BB.  */
+           if (worklist.is_empty ()
+               || gimple_call_arg (worklist.last (), 2) != mask
+               || !is_valid_sink (worklist.last (), last_store))

as I understand the code the last check is redundant with the invariant
you track if you verify the stmt you breaked from the inner loop is
actually equal to worklist.last () and you add a flag to track whether
you did visit a load (vuse) in the sinking loop you didn't end up sinking.

+             /* Issue different messages depending on FIRST_DUMP.  */
+             if (first_dump)
+               {
+                 dump_printf_loc (MSG_NOTE, vect_location,
+                                  "Move MASK_STORE to new bb#%d\n",
+                              store_bb->index);
+                 first_dump = false;
+               }
+             else
+               dump_printf_loc (MSG_NOTE, vect_location,
+                                "Move MASK_STORE to created bb\n");

just add a separate dump when you create the BB, "Created new bb#%d for ..."
to avoid this.

Note that I can't comment on the x86 backend part so that will need to
be reviewed by somebody
else.

Thanks,
Richard.

> Thanks.
> Yuri.
>
> 2016-01-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR middle-end/68542
> * config/i386/i386.c (ix86_expand_branch): Implement integral vector
> comparison with boolean result.
> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
> for vector comparion with eq/ne only.
> * tree-vect-loop.c (is_valid_sink): New function.
> (optimize_mask_stores): Likewise.
> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
> has_mask_store field of vect_info.
> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
> vectorized loops having masked stores.
> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
> correspondent macros.
> (optimize_mask_stores): Add prototype.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
>
> 2016-01-18 17:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>> Thanks Richard.
>>>
>>> I changed the check on type as you proposed.
>>>
>>> What about the second back-end part of patch (it has been sent 08.12.15).
>>
>> Can't see it in my inbox - can you reply to the mail with a ping?
>>
>> Thanks,
>> Richard.
>>
>>> Thanks.
>>> Yuri.
>>>
>>> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>> Hi Richard,
>>>>>
>>>>> Did you have anu chance to look at updated patch?
>>>>
>>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>>> index acbb70b..208a752 100644
>>>> --- a/gcc/tree-vrp.c
>>>> +++ b/gcc/tree-vrp.c
>>>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>>>> gimple_stmt_iterator si,
>>>>                                                 &comp_code, &val))
>>>>      return;
>>>>
>>>> +  /* VRP doesn't track ranges for vector types.  */
>>>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>>>> +    return;
>>>> +
>>>>
>>>> please instead fix extract_code_and_val_from_cond_with_ops with
>>>>
>>>> Index: gcc/tree-vrp.c
>>>> ===================================================================
>>>> --- gcc/tree-vrp.c      (revision 232506)
>>>> +++ gcc/tree-vrp.c      (working copy)
>>>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>>>    if (invert)
>>>>      comp_code = invert_tree_comparison (comp_code, 0);
>>>>
>>>> -  /* VRP does not handle float types.  */
>>>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>>>> +  /* VRP only handles integral and pointer types.  */
>>>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>>>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>>>      return false;
>>>>
>>>>    /* Do not register always-false predicates.
>>>>
>>>> Ok with that change.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks.
>>>>> Yuri.
>>>>>
>>>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>>>> Hi Richard,
>>>>>>
>>>>>> Here is updated patch for middle-end part of the whole patch which
>>>>>> fixes all your remarks I hope.
>>>>>>
>>>>>> Regression testing and bootstrapping did not show any new failures.
>>>>>> Is it OK for trunk?
>>>>>>
>>>>>> Yuri.
>>>>>>
>>>>>> ChangeLog:
>>>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>>
>>>>>> PR middle-end/68542
>>>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>>>> of mixind vector and scalar types.
>>>>>> (fold_relational_const): Add handling of vector
>>>>>> comparison with boolean result.
>>>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>>>> (verify_gimple_cond): Likewise.
>>>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Yuri Rumyantsev Jan. 22, 2016, 2:29 p.m. UTC | #5
Richard,

I fixed all remarks pointed by you in vectorizer part of patch. Could
you take a look on modified patch.

Uros,

Could you please review i386 part of patch related to support of
conditional branches with vector comparison.

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

Thanks.
Yuri.

ChangeLog:

2016-01-22  Yuri Rumyantsev  <ysrumyan@gmail.com>

PR middle-end/68542
* config/i386/i386.c (ix86_expand_branch): Add support for conditional
brnach with vector comparison.
* config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
for vector comparion with eq/ne only.
(optimize_mask_stores): New function.
* tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
has_mask_store field of vect_info.
* tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
vectorized loops having masked stores after vec_info destroy.
* tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
correspondent macros.
(optimize_mask_stores): Add prototype.

gcc/testsuite/ChangeLog:
* gcc.dg/vect/vect-mask-store-move-1.c: New test.
* testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c: New test.

2016-01-20 15:24 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Mon, Jan 18, 2016 at 3:50 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Richard,
>>
>> Here is the second part of patch which really preforms mask stores and
>> all statements related to it to new basic block guarded by test on
>> zero mask. Hew test is also added.
>>
>> Is it OK for trunk?
>
> +  /* Pick up all masked stores in loop if any.  */
> +  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +    {
> +      stmt = gsi_stmt (gsi);
>
> you fail to iterate over all BBs of the loop here.  Please follow
> other uses in the
> vectorizer.
>
> +  while (!worklist.is_empty ())
> +    {
> +      gimple *last, *last_store;
> +      edge e, efalse;
> +      tree mask;
> +      basic_block store_bb, join_bb;
> +      gimple_stmt_iterator gsi_to;
> +      /* tree arg3; */
>
> remove
>
> +      tree vdef, new_vdef;
> +      gphi *phi;
> +      bool first_dump;
> +      tree vectype;
> +      tree zero;
> +
> +      last = worklist.pop ();
> +      mask = gimple_call_arg (last, 2);
> +      /* Create new bb.  */
>
> bb should be initialized from gimple_bb (last), not loop->header
>
> +      e = split_block (bb, last);
>
> +               gsi_from = gsi_for_stmt (stmt1);
> +               gsi_to = gsi_start_bb (store_bb);
> +               gsi_move_before (&gsi_from, &gsi_to);
> +               update_stmt (stmt1);
>
> I think the update_stmt is redundant and you should be able to
> keep two gsis throughout the loop, from and to, no?
>
> +           /* Put other masked stores with the same mask to STORE_BB.  */
> +           if (worklist.is_empty ()
> +               || gimple_call_arg (worklist.last (), 2) != mask
> +               || !is_valid_sink (worklist.last (), last_store))
>
> as I understand the code the last check is redundant with the invariant
> you track if you verify the stmt you breaked from the inner loop is
> actually equal to worklist.last () and you add a flag to track whether
> you did visit a load (vuse) in the sinking loop you didn't end up sinking.
>
> +             /* Issue different messages depending on FIRST_DUMP.  */
> +             if (first_dump)
> +               {
> +                 dump_printf_loc (MSG_NOTE, vect_location,
> +                                  "Move MASK_STORE to new bb#%d\n",
> +                              store_bb->index);
> +                 first_dump = false;
> +               }
> +             else
> +               dump_printf_loc (MSG_NOTE, vect_location,
> +                                "Move MASK_STORE to created bb\n");
>
> just add a separate dump when you create the BB, "Created new bb#%d for ..."
> to avoid this.
>
> Note that I can't comment on the x86 backend part so that will need to
> be reviewed by somebody
> else.
>
> Thanks,
> Richard.
>
>> Thanks.
>> Yuri.
>>
>> 2016-01-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR middle-end/68542
>> * config/i386/i386.c (ix86_expand_branch): Implement integral vector
>> comparison with boolean result.
>> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
>> for vector comparion with eq/ne only.
>> * tree-vect-loop.c (is_valid_sink): New function.
>> (optimize_mask_stores): Likewise.
>> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
>> has_mask_store field of vect_info.
>> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
>> vectorized loops having masked stores.
>> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
>> correspondent macros.
>> (optimize_mask_stores): Add prototype.
>>
>> gcc/testsuite/ChangeLog:
>> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
>>
>> 2016-01-18 17:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>> Thanks Richard.
>>>>
>>>> I changed the check on type as you proposed.
>>>>
>>>> What about the second back-end part of patch (it has been sent 08.12.15).
>>>
>>> Can't see it in my inbox - can you reply to the mail with a ping?
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks.
>>>> Yuri.
>>>>
>>>> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>>> Hi Richard,
>>>>>>
>>>>>> Did you have anu chance to look at updated patch?
>>>>>
>>>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>>>> index acbb70b..208a752 100644
>>>>> --- a/gcc/tree-vrp.c
>>>>> +++ b/gcc/tree-vrp.c
>>>>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>>>>> gimple_stmt_iterator si,
>>>>>                                                 &comp_code, &val))
>>>>>      return;
>>>>>
>>>>> +  /* VRP doesn't track ranges for vector types.  */
>>>>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>>>>> +    return;
>>>>> +
>>>>>
>>>>> please instead fix extract_code_and_val_from_cond_with_ops with
>>>>>
>>>>> Index: gcc/tree-vrp.c
>>>>> ===================================================================
>>>>> --- gcc/tree-vrp.c      (revision 232506)
>>>>> +++ gcc/tree-vrp.c      (working copy)
>>>>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>>>>    if (invert)
>>>>>      comp_code = invert_tree_comparison (comp_code, 0);
>>>>>
>>>>> -  /* VRP does not handle float types.  */
>>>>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>>>>> +  /* VRP only handles integral and pointer types.  */
>>>>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>>>>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>>>>      return false;
>>>>>
>>>>>    /* Do not register always-false predicates.
>>>>>
>>>>> Ok with that change.
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> Thanks.
>>>>>> Yuri.
>>>>>>
>>>>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>> Here is updated patch for middle-end part of the whole patch which
>>>>>>> fixes all your remarks I hope.
>>>>>>>
>>>>>>> Regression testing and bootstrapping did not show any new failures.
>>>>>>> Is it OK for trunk?
>>>>>>>
>>>>>>> Yuri.
>>>>>>>
>>>>>>> ChangeLog:
>>>>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>>>
>>>>>>> PR middle-end/68542
>>>>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>>>>> of mixind vector and scalar types.
>>>>>>> (fold_relational_const): Add handling of vector
>>>>>>> comparison with boolean result.
>>>>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>>>>> (verify_gimple_cond): Likewise.
>>>>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
H.J. Lu Jan. 22, 2016, 2:50 p.m. UTC | #6
On Fri, Jan 22, 2016 at 6:29 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Richard,
>
> I fixed all remarks pointed by you in vectorizer part of patch. Could
> you take a look on modified patch.
>
> Uros,
>
> Could you please review i386 part of patch related to support of
> conditional branches with vector comparison.
>
> Bootstrap and regression testing did not show any new failures.
> Is it OK for trunk?
>
> Thanks.
> Yuri.
>
> ChangeLog:
>
> 2016-01-22  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR middle-end/68542
> * config/i386/i386.c (ix86_expand_branch): Add support for conditional
> brnach with vector comparison.
  ^^^^^^^^^ Typo.
> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
> for vector comparion with eq/ne only.
> (optimize_mask_stores): New function.
> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
> has_mask_store field of vect_info.
> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
> vectorized loops having masked stores after vec_info destroy.
> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
> correspondent macros.
> (optimize_mask_stores): Add prototype.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
> * testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c: New test.
>
Richard Biener Jan. 28, 2016, 1:26 p.m. UTC | #7
On Fri, Jan 22, 2016 at 3:29 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Richard,
>
> I fixed all remarks pointed by you in vectorizer part of patch. Could
> you take a look on modified patch.

+               if (is_gimple_call (stmt1))
+                 lhs = gimple_call_lhs (stmt1);
+               else
+                 if (gimple_code (stmt1) == GIMPLE_ASSIGN)
+                   lhs = gimple_assign_lhs (stmt1);
+                 else
+                   break;

use

  lhs = gimple_get_lhs (stmt1);
  if (!lhs)
    break;

you forget to free bbs, I suggest to do it after seeding the worklist.

Ok with those changes if the backend changes are approved.

Sorry for the delay in reviewing this.

Thanks,
Richard.

> Uros,
>
> Could you please review i386 part of patch related to support of
> conditional branches with vector comparison.
>
> Bootstrap and regression testing did not show any new failures.
> Is it OK for trunk?
>
> Thanks.
> Yuri.
>
> ChangeLog:
>
> 2016-01-22  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR middle-end/68542
> * config/i386/i386.c (ix86_expand_branch): Add support for conditional
> brnach with vector comparison.
> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
> for vector comparion with eq/ne only.
> (optimize_mask_stores): New function.
> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
> has_mask_store field of vect_info.
> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
> vectorized loops having masked stores after vec_info destroy.
> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
> correspondent macros.
> (optimize_mask_stores): Add prototype.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
> * testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c: New test.
>
> 2016-01-20 15:24 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Mon, Jan 18, 2016 at 3:50 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>> Richard,
>>>
>>> Here is the second part of patch which really preforms mask stores and
>>> all statements related to it to new basic block guarded by test on
>>> zero mask. Hew test is also added.
>>>
>>> Is it OK for trunk?
>>
>> +  /* Pick up all masked stores in loop if any.  */
>> +  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +    {
>> +      stmt = gsi_stmt (gsi);
>>
>> you fail to iterate over all BBs of the loop here.  Please follow
>> other uses in the
>> vectorizer.
>>
>> +  while (!worklist.is_empty ())
>> +    {
>> +      gimple *last, *last_store;
>> +      edge e, efalse;
>> +      tree mask;
>> +      basic_block store_bb, join_bb;
>> +      gimple_stmt_iterator gsi_to;
>> +      /* tree arg3; */
>>
>> remove
>>
>> +      tree vdef, new_vdef;
>> +      gphi *phi;
>> +      bool first_dump;
>> +      tree vectype;
>> +      tree zero;
>> +
>> +      last = worklist.pop ();
>> +      mask = gimple_call_arg (last, 2);
>> +      /* Create new bb.  */
>>
>> bb should be initialized from gimple_bb (last), not loop->header
>>
>> +      e = split_block (bb, last);
>>
>> +               gsi_from = gsi_for_stmt (stmt1);
>> +               gsi_to = gsi_start_bb (store_bb);
>> +               gsi_move_before (&gsi_from, &gsi_to);
>> +               update_stmt (stmt1);
>>
>> I think the update_stmt is redundant and you should be able to
>> keep two gsis throughout the loop, from and to, no?
>>
>> +           /* Put other masked stores with the same mask to STORE_BB.  */
>> +           if (worklist.is_empty ()
>> +               || gimple_call_arg (worklist.last (), 2) != mask
>> +               || !is_valid_sink (worklist.last (), last_store))
>>
>> as I understand the code the last check is redundant with the invariant
>> you track if you verify the stmt you breaked from the inner loop is
>> actually equal to worklist.last () and you add a flag to track whether
>> you did visit a load (vuse) in the sinking loop you didn't end up sinking.
>>
>> +             /* Issue different messages depending on FIRST_DUMP.  */
>> +             if (first_dump)
>> +               {
>> +                 dump_printf_loc (MSG_NOTE, vect_location,
>> +                                  "Move MASK_STORE to new bb#%d\n",
>> +                              store_bb->index);
>> +                 first_dump = false;
>> +               }
>> +             else
>> +               dump_printf_loc (MSG_NOTE, vect_location,
>> +                                "Move MASK_STORE to created bb\n");
>>
>> just add a separate dump when you create the BB, "Created new bb#%d for ..."
>> to avoid this.
>>
>> Note that I can't comment on the x86 backend part so that will need to
>> be reviewed by somebody
>> else.
>>
>> Thanks,
>> Richard.
>>
>>> Thanks.
>>> Yuri.
>>>
>>> 2016-01-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>
>>> PR middle-end/68542
>>> * config/i386/i386.c (ix86_expand_branch): Implement integral vector
>>> comparison with boolean result.
>>> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
>>> for vector comparion with eq/ne only.
>>> * tree-vect-loop.c (is_valid_sink): New function.
>>> (optimize_mask_stores): Likewise.
>>> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
>>> has_mask_store field of vect_info.
>>> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
>>> vectorized loops having masked stores.
>>> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
>>> correspondent macros.
>>> (optimize_mask_stores): Add prototype.
>>>
>>> gcc/testsuite/ChangeLog:
>>> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
>>>
>>> 2016-01-18 17:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>> On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>> Thanks Richard.
>>>>>
>>>>> I changed the check on type as you proposed.
>>>>>
>>>>> What about the second back-end part of patch (it has been sent 08.12.15).
>>>>
>>>> Can't see it in my inbox - can you reply to the mail with a ping?
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks.
>>>>> Yuri.
>>>>>
>>>>> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>>>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>> Did you have anu chance to look at updated patch?
>>>>>>
>>>>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>>>>> index acbb70b..208a752 100644
>>>>>> --- a/gcc/tree-vrp.c
>>>>>> +++ b/gcc/tree-vrp.c
>>>>>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>>>>>> gimple_stmt_iterator si,
>>>>>>                                                 &comp_code, &val))
>>>>>>      return;
>>>>>>
>>>>>> +  /* VRP doesn't track ranges for vector types.  */
>>>>>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>>>>>> +    return;
>>>>>> +
>>>>>>
>>>>>> please instead fix extract_code_and_val_from_cond_with_ops with
>>>>>>
>>>>>> Index: gcc/tree-vrp.c
>>>>>> ===================================================================
>>>>>> --- gcc/tree-vrp.c      (revision 232506)
>>>>>> +++ gcc/tree-vrp.c      (working copy)
>>>>>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>>>>>    if (invert)
>>>>>>      comp_code = invert_tree_comparison (comp_code, 0);
>>>>>>
>>>>>> -  /* VRP does not handle float types.  */
>>>>>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>>>>>> +  /* VRP only handles integral and pointer types.  */
>>>>>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>>>>>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>>>>>      return false;
>>>>>>
>>>>>>    /* Do not register always-false predicates.
>>>>>>
>>>>>> Ok with that change.
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> Thanks.
>>>>>>> Yuri.
>>>>>>>
>>>>>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>>>>>> Hi Richard,
>>>>>>>>
>>>>>>>> Here is updated patch for middle-end part of the whole patch which
>>>>>>>> fixes all your remarks I hope.
>>>>>>>>
>>>>>>>> Regression testing and bootstrapping did not show any new failures.
>>>>>>>> Is it OK for trunk?
>>>>>>>>
>>>>>>>> Yuri.
>>>>>>>>
>>>>>>>> ChangeLog:
>>>>>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>>>>
>>>>>>>> PR middle-end/68542
>>>>>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>>>>>> of mixind vector and scalar types.
>>>>>>>> (fold_relational_const): Add handling of vector
>>>>>>>> comparison with boolean result.
>>>>>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>>>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>>>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>>>>>> (verify_gimple_cond): Likewise.
>>>>>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Yuri Rumyantsev Jan. 28, 2016, 1:37 p.m. UTC | #8
Thanks Richard.

Uros,

Could you please review back-end part of this patch?

Thanks.
Yuri.

2016-01-28 16:26 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Fri, Jan 22, 2016 at 3:29 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Richard,
>>
>> I fixed all remarks pointed by you in vectorizer part of patch. Could
>> you take a look on modified patch.
>
> +               if (is_gimple_call (stmt1))
> +                 lhs = gimple_call_lhs (stmt1);
> +               else
> +                 if (gimple_code (stmt1) == GIMPLE_ASSIGN)
> +                   lhs = gimple_assign_lhs (stmt1);
> +                 else
> +                   break;
>
> use
>
>   lhs = gimple_get_lhs (stmt1);
>   if (!lhs)
>     break;
>
> you forget to free bbs, I suggest to do it after seeding the worklist.
>
> Ok with those changes if the backend changes are approved.
>
> Sorry for the delay in reviewing this.
>
> Thanks,
> Richard.
>
>> Uros,
>>
>> Could you please review i386 part of patch related to support of
>> conditional branches with vector comparison.
>>
>> Bootstrap and regression testing did not show any new failures.
>> Is it OK for trunk?
>>
>> Thanks.
>> Yuri.
>>
>> ChangeLog:
>>
>> 2016-01-22  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR middle-end/68542
>> * config/i386/i386.c (ix86_expand_branch): Add support for conditional
>> brnach with vector comparison.
>> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
>> for vector comparion with eq/ne only.
>> (optimize_mask_stores): New function.
>> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
>> has_mask_store field of vect_info.
>> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
>> vectorized loops having masked stores after vec_info destroy.
>> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
>> correspondent macros.
>> (optimize_mask_stores): Add prototype.
>>
>> gcc/testsuite/ChangeLog:
>> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
>> * testsuite/gcc.target/i386/avx2-vect-mask-store-move1.c: New test.
>>
>> 2016-01-20 15:24 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Mon, Jan 18, 2016 at 3:50 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>> Richard,
>>>>
>>>> Here is the second part of patch which really preforms mask stores and
>>>> all statements related to it to new basic block guarded by test on
>>>> zero mask. Hew test is also added.
>>>>
>>>> Is it OK for trunk?
>>>
>>> +  /* Pick up all masked stores in loop if any.  */
>>> +  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>> +    {
>>> +      stmt = gsi_stmt (gsi);
>>>
>>> you fail to iterate over all BBs of the loop here.  Please follow
>>> other uses in the
>>> vectorizer.
>>>
>>> +  while (!worklist.is_empty ())
>>> +    {
>>> +      gimple *last, *last_store;
>>> +      edge e, efalse;
>>> +      tree mask;
>>> +      basic_block store_bb, join_bb;
>>> +      gimple_stmt_iterator gsi_to;
>>> +      /* tree arg3; */
>>>
>>> remove
>>>
>>> +      tree vdef, new_vdef;
>>> +      gphi *phi;
>>> +      bool first_dump;
>>> +      tree vectype;
>>> +      tree zero;
>>> +
>>> +      last = worklist.pop ();
>>> +      mask = gimple_call_arg (last, 2);
>>> +      /* Create new bb.  */
>>>
>>> bb should be initialized from gimple_bb (last), not loop->header
>>>
>>> +      e = split_block (bb, last);
>>>
>>> +               gsi_from = gsi_for_stmt (stmt1);
>>> +               gsi_to = gsi_start_bb (store_bb);
>>> +               gsi_move_before (&gsi_from, &gsi_to);
>>> +               update_stmt (stmt1);
>>>
>>> I think the update_stmt is redundant and you should be able to
>>> keep two gsis throughout the loop, from and to, no?
>>>
>>> +           /* Put other masked stores with the same mask to STORE_BB.  */
>>> +           if (worklist.is_empty ()
>>> +               || gimple_call_arg (worklist.last (), 2) != mask
>>> +               || !is_valid_sink (worklist.last (), last_store))
>>>
>>> as I understand the code the last check is redundant with the invariant
>>> you track if you verify the stmt you breaked from the inner loop is
>>> actually equal to worklist.last () and you add a flag to track whether
>>> you did visit a load (vuse) in the sinking loop you didn't end up sinking.
>>>
>>> +             /* Issue different messages depending on FIRST_DUMP.  */
>>> +             if (first_dump)
>>> +               {
>>> +                 dump_printf_loc (MSG_NOTE, vect_location,
>>> +                                  "Move MASK_STORE to new bb#%d\n",
>>> +                              store_bb->index);
>>> +                 first_dump = false;
>>> +               }
>>> +             else
>>> +               dump_printf_loc (MSG_NOTE, vect_location,
>>> +                                "Move MASK_STORE to created bb\n");
>>>
>>> just add a separate dump when you create the BB, "Created new bb#%d for ..."
>>> to avoid this.
>>>
>>> Note that I can't comment on the x86 backend part so that will need to
>>> be reviewed by somebody
>>> else.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks.
>>>> Yuri.
>>>>
>>>> 2016-01-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>
>>>> PR middle-end/68542
>>>> * config/i386/i386.c (ix86_expand_branch): Implement integral vector
>>>> comparison with boolean result.
>>>> * config/i386/sse.md (define_expand "cbranch<mode>4): Add define-expand
>>>> for vector comparion with eq/ne only.
>>>> * tree-vect-loop.c (is_valid_sink): New function.
>>>> (optimize_mask_stores): Likewise.
>>>> * tree-vect-stmts.c (vectorizable_mask_load_store): Initialize
>>>> has_mask_store field of vect_info.
>>>> * tree-vectorizer.c (vectorize_loops): Invoke optimaze_mask_stores for
>>>> vectorized loops having masked stores.
>>>> * tree-vectorizer.h (loop_vec_info): Add new has_mask_store field and
>>>> correspondent macros.
>>>> (optimize_mask_stores): Add prototype.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>> * gcc.dg/vect/vect-mask-store-move-1.c: New test.
>>>>
>>>> 2016-01-18 17:07 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>>> On Mon, Jan 18, 2016 at 3:02 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>>> Thanks Richard.
>>>>>>
>>>>>> I changed the check on type as you proposed.
>>>>>>
>>>>>> What about the second back-end part of patch (it has been sent 08.12.15).
>>>>>
>>>>> Can't see it in my inbox - can you reply to the mail with a ping?
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> Thanks.
>>>>>> Yuri.
>>>>>>
>>>>>> 2016-01-18 15:44 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>>>>> On Mon, Jan 11, 2016 at 11:06 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>>>>> Hi Richard,
>>>>>>>>
>>>>>>>> Did you have anu chance to look at updated patch?
>>>>>>>
>>>>>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>>>>>> index acbb70b..208a752 100644
>>>>>>> --- a/gcc/tree-vrp.c
>>>>>>> +++ b/gcc/tree-vrp.c
>>>>>>> @@ -5771,6 +5771,10 @@ register_edge_assert_for (tree name, edge e,
>>>>>>> gimple_stmt_iterator si,
>>>>>>>                                                 &comp_code, &val))
>>>>>>>      return;
>>>>>>>
>>>>>>> +  /* VRP doesn't track ranges for vector types.  */
>>>>>>> +  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
>>>>>>> +    return;
>>>>>>> +
>>>>>>>
>>>>>>> please instead fix extract_code_and_val_from_cond_with_ops with
>>>>>>>
>>>>>>> Index: gcc/tree-vrp.c
>>>>>>> ===================================================================
>>>>>>> --- gcc/tree-vrp.c      (revision 232506)
>>>>>>> +++ gcc/tree-vrp.c      (working copy)
>>>>>>> @@ -5067,8 +5067,9 @@ extract_code_and_val_from_cond_with_ops
>>>>>>>    if (invert)
>>>>>>>      comp_code = invert_tree_comparison (comp_code, 0);
>>>>>>>
>>>>>>> -  /* VRP does not handle float types.  */
>>>>>>> -  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
>>>>>>> +  /* VRP only handles integral and pointer types.  */
>>>>>>> +  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
>>>>>>> +      && ! POINTER_TYPE_P (TREE_TYPE (val)))
>>>>>>>      return false;
>>>>>>>
>>>>>>>    /* Do not register always-false predicates.
>>>>>>>
>>>>>>> Ok with that change.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Yuri.
>>>>>>>>
>>>>>>>> 2015-12-18 13:20 GMT+03:00 Yuri Rumyantsev <ysrumyan@gmail.com>:
>>>>>>>>> Hi Richard,
>>>>>>>>>
>>>>>>>>> Here is updated patch for middle-end part of the whole patch which
>>>>>>>>> fixes all your remarks I hope.
>>>>>>>>>
>>>>>>>>> Regression testing and bootstrapping did not show any new failures.
>>>>>>>>> Is it OK for trunk?
>>>>>>>>>
>>>>>>>>> Yuri.
>>>>>>>>>
>>>>>>>>> ChangeLog:
>>>>>>>>> 2015-12-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>>>>>
>>>>>>>>> PR middle-end/68542
>>>>>>>>> * fold-const.c (fold_binary_op_with_conditional_arg): Bail out for case
>>>>>>>>> of mixind vector and scalar types.
>>>>>>>>> (fold_relational_const): Add handling of vector
>>>>>>>>> comparison with boolean result.
>>>>>>>>> * tree-cfg.c (verify_gimple_comparison): Add argument CODE, allow
>>>>>>>>> comparison of vector operands with boolean result for EQ/NE only.
>>>>>>>>> (verify_gimple_assign_binary): Adjust call for verify_gimple_comparison.
>>>>>>>>> (verify_gimple_cond): Likewise.
>>>>>>>>> * tree-ssa-forwprop.c (combine_cond_expr_cond): Do not perform
Uros Bizjak Jan. 28, 2016, 2:24 p.m. UTC | #9
On Thu, Jan 28, 2016 at 2:37 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Thanks Richard.
>
> Uros,
>
> Could you please review back-end part of this patch?

No problem, but please in future CC me on the x86 patches, so I won't forgot.

+(define_expand "cbranch<mode>4"
+  [(set (reg:CC FLAGS_REG)
+    (compare:CC (match_operand:V48_AVX2 1 "nonimmediate_operand")
+            (match_operand:V48_AVX2 2 "register_operand")))
+

Please swap predicates, operand 1 should be register_operand and
operand 2 nonimmediate operand.

+   (set (pc) (if_then_else
+           (match_operator 0 "bt_comparison_operator"
+        [(reg:CC FLAGS_REG) (const_int 0)])
+           (label_ref (match_operand 3))
+           (pc)))]
+  "TARGET_AVX2"

PTEST was introduced with SSE4_1, I see no reason to limit this
transformation to AVX2. However, since you check MODE_VECTOR_INT in
the expander, it looks that the pattern should only handle integer
modes.

+  /* Handle special case - vector comparsion with boolean result, transform
+     it using ptest instruction.  */
+  if (GET_MODE_CLASS (mode) == MODE_VECTOR_INT)
+    {
+      rtx lhs;

No need for the above variable, we will use tmp here.

+      rtx flag;
+      machine_mode p_mode = GET_MODE_SIZE (mode) == 32 ? V4DImode : V2DImode;

space here.

+      gcc_assert (code == EQ || code == NE);

+      if (!REG_P (op0))
+    op0 = force_reg (mode, op0);
+      if (!REG_P (op1))
+    op1 = force_reg (mode, op1);

No need for the above fixups, predicates already did the job.

+      /* Generate subtraction since we can't check that one operand is
+     zero vector.  */
+      lhs = gen_reg_rtx (mode);

tmp = ...

+      emit_insn (gen_rtx_SET (lhs, gen_rtx_MINUS (mode, op0, op1)));

Since we are only interested in equality, should we rather use XOR,
since it is commutative operator.

+      lhs = gen_rtx_SUBREG (p_mode, lhs, 0);

tmp = gen_lowpart (p_mode, tmp);

+      tmp = gen_rtx_SET (gen_rtx_REG (CCmode, FLAGS_REG),
+             gen_rtx_UNSPEC (CCmode,
+                     gen_rtvec (2, lhs, lhs),
+                     UNSPEC_PTEST));
+      emit_insn (tmp);

There is no need for a temporary here, just use

emit_insn (gen_rtx_SET ( ... ) ...

+      tmp = gen_rtx_fmt_ee (code, VOIDmode, flag, const0_rtx);
+      if (code == EQ)
+    tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, tmp,
+                    gen_rtx_LABEL_REF (VOIDmode, label), pc_rtx);
+      else
+    tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, tmp,
+                    pc_rtx, gen_rtx_LABEL_REF (VOIDmode, label));
+      emit_jump_insn (gen_rtx_SET (pc_rtx, tmp));
+      return;

Oh.... please use something involving std::swap here.

Uros.
diff mbox

Patch

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index acbb70b..208a752 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -5771,6 +5771,10 @@  register_edge_assert_for (tree name, edge e,
gimple_stmt_iterator si,
                                                &comp_code, &val))
     return;

+  /* VRP doesn't track ranges for vector types.  */
+  if (TREE_CODE (TREE_TYPE (name)) == VECTOR_TYPE)
+    return;
+

please instead fix extract_code_and_val_from_cond_with_ops with

Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c      (revision 232506)
+++ gcc/tree-vrp.c      (working copy)
@@ -5067,8 +5067,9 @@  extract_code_and_val_from_cond_with_ops
   if (invert)
     comp_code = invert_tree_comparison (comp_code, 0);

-  /* VRP does not handle float types.  */
-  if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (val)))
+  /* VRP only handles integral and pointer types.  */
+  if (! INTEGRAL_TYPE_P (TREE_TYPE (val))
+      && ! POINTER_TYPE_P (TREE_TYPE (val)))
     return false;

   /* Do not register always-false predicates.