From: Yuri Rumyantsev <ysrumyan@gmail.com>
To: Richard Biener <richard.guenther@gmail.com>,
Uros Bizjak <ubizjak@gmail.com>
Cc: gcc-patches <gcc-patches@gcc.gnu.org>,
Igor Zamyatin <izamyatin@gmail.com>,
Kirill Yukhin <kirill.Yukhin@gmail.com>
Subject: Re: [PATCH PR68542]
Date: Thu, 28 Jan 2016 13:37:00 -0000 [thread overview]
Message-ID: <CAEoMCqTMVbb5c5cTLg=_NUN=VHaO=E=ehK+N0VuCX0_e8p4V8g@mail.gmail.com> (raw)
In-Reply-To: <CAFiYyc1GYRCwGnpkVNyqSmGBm-mf818Ro0FpurMJCyVnwg7kXw@mail.gmail.com>
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
next prev parent reply other threads:[~2016-01-28 13:37 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-30 13:24 Yuri Rumyantsev
2015-12-04 12:18 ` Richard Biener
2015-12-04 15:07 ` Yuri Rumyantsev
2015-12-07 10:57 ` Yuri Rumyantsev
2015-12-08 12:34 ` Yuri Rumyantsev
2015-12-10 13:36 ` Richard Biener
2015-12-11 14:03 ` Yuri Rumyantsev
2015-12-16 13:37 ` Richard Biener
2015-12-18 10:20 ` Yuri Rumyantsev
2016-01-11 10:06 ` Yuri Rumyantsev
2016-01-18 12:44 ` Richard Biener
2016-01-18 14:02 ` Yuri Rumyantsev
2016-01-18 14:07 ` Richard Biener
2016-01-18 14:50 ` Yuri Rumyantsev
2016-01-20 12:25 ` Richard Biener
2016-01-22 14:29 ` Yuri Rumyantsev
2016-01-22 14:50 ` H.J. Lu
2016-01-28 13:26 ` Richard Biener
2016-01-28 13:37 ` Yuri Rumyantsev [this message]
2016-01-28 14:24 ` Uros Bizjak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEoMCqTMVbb5c5cTLg=_NUN=VHaO=E=ehK+N0VuCX0_e8p4V8g@mail.gmail.com' \
--to=ysrumyan@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=izamyatin@gmail.com \
--cc=kirill.Yukhin@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).