Re: [PATCH PR68542]

[Richard Biener <richard dot guenther at gmail dot 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