[PATCH] vectorizer: add _bb_vec_info::const_iterator
Martin Liška
mliska@suse.cz
Wed Jun 17 17:05:52 GMT 2020
On 6/16/20 4:14 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:
>> On 6/16/20 10:50 AM, Richard Sandiford wrote:
>>> Martin Liška <mliska@suse.cz> writes:
>>>>> Also, one minor formatting nit, sorry: the other functions instead
>>>>> indent the “{” block by the same amount as the function prototype,
>>>>> which seems more consistent with the usual out-of-class formatting.
>>>>
>>>> Hope I fixed that.
>>>
>>> Sorry, I meant the other functions were IMO formatted correctly, with the
>>> “{” directly under the function name. It looks like the new patch instead
>>> indents all “{” by two spaces relative to the function name or “struct”
>>> keyword. I.e. IMO:
>>>
>>> struct const_iterator
>>> {
>>> };
>>>
>>> seems better than:
>>>
>>> struct const_iterator
>>> {
>>> };
>>>
>>> and:
>>>
>>> const const_iterator &
>>> operator++ ()
>>> {
>>> }
>>>
>>> seems better than:
>>>
>>> const const_iterator &
>>> operator++ ()
>>> {
>>> }
>>>
>>> This makes the formatting consistent with definitions in column 0.
>>>
>>>> About rbiener's comment, I wrapper the iterators with bb_vinfo::region_stmts..
>>>
>>> Sorry for dragging this on longer, but…
>>>
>>>> @@ -5120,20 +5120,14 @@ vect_determine_precisions (vec_info *vinfo)
>>>> else
>>>> {
>>>> bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
>>>> - gimple_stmt_iterator si = bb_vinfo->region_end;
>>>> - gimple *stmt;
>>>> - do
>>>> + for ( _bb_vec_info::reverse_iterator it = bb_vinfo->region_stmts.rbegin ();
>>>> + it != bb_vinfo->region_stmts.rend (); ++it)
>>>> {
>>>> - if (!gsi_stmt (si))
>>>> - si = gsi_last_bb (bb_vinfo->bb);
>>>> - else
>>>> - gsi_prev (&si);
>>>> - stmt = gsi_stmt (si);
>>>> + gimple *stmt = *it;
>>>> stmt_vec_info stmt_info = vinfo->lookup_stmt (stmt);
>>>> if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
>>>> vect_determine_stmt_precisions (vinfo, stmt_info);
>>>> }
>>>> - while (stmt != gsi_stmt (bb_vinfo->region_begin));
>>>> }
>>>> }
>>>
>>> I think this should be a similar style, i.e.
>>>
>>> for (gimple *stmt : bb_vinfo->reverse_region_stmts ())
>>>
>>> rather than using iterators directly.
>>>
>>>> @@ -5492,10 +5486,8 @@ vect_pattern_recog (vec_info *vinfo)
>>>> else
>>>> {
>>>> bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
>>>> - for (si = bb_vinfo->region_begin;
>>>> - gsi_stmt (si) != gsi_stmt (bb_vinfo->region_end); gsi_next (&si))
>>>> + for (gimple *stmt : bb_vinfo->region_stmts)
>>>> {
>>>> - gimple *stmt = gsi_stmt (si);
>>>> stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
>>>> if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
>>>> continue;
>>>> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
>>>> index 303410c2fc4..f4809c2207d 100644
>>>> --- a/gcc/tree-vect-slp.c
>>>> +++ b/gcc/tree-vect-slp.c
>>>> @@ -2546,20 +2546,15 @@ vect_detect_hybrid_slp (loop_vec_info loop_vinfo)
>>>> /* Initialize a bb_vec_info struct for the statements between
>>>> REGION_BEGIN_IN (inclusive) and REGION_END_IN (exclusive). */
>>>>
>>>> -_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
>>>> - gimple_stmt_iterator region_end_in,
>>>> +_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin,
>>>> + gimple_stmt_iterator region_end,
>>>> vec_info_shared *shared)
>>>> : vec_info (vec_info::bb, init_cost (NULL), shared),
>>>> - bb (gsi_bb (region_begin_in)),
>>>> - region_begin (region_begin_in),
>>>> - region_end (region_end_in)
>>>> + bb (gsi_bb (region_begin)),
>>>> + region_stmts (region_begin, region_end)
>>>> {
>>>> - gimple_stmt_iterator gsi;
>>>> -
>>>> - for (gsi = region_begin; gsi_stmt (gsi) != gsi_stmt (region_end);
>>>> - gsi_next (&gsi))
>>>> + for (gimple *stmt : this->region_stmts)
>>>> {
>>>> - gimple *stmt = gsi_stmt (gsi);
>>>> gimple_set_uid (stmt, 0);
>>>> if (is_gimple_debug (stmt))
>>>> continue;
>>>> @@ -2575,10 +2570,9 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
>>>>
>>>> _bb_vec_info::~_bb_vec_info ()
>>>> {
>>>> - for (gimple_stmt_iterator si = region_begin;
>>>> - gsi_stmt (si) != gsi_stmt (region_end); gsi_next (&si))
>>>> + for (gimple *stmt : this->region_stmts)
>>>> /* Reset region marker. */
>>>> - gimple_set_uid (gsi_stmt (si), -1);
>>>> + gimple_set_uid (stmt, -1);
>>>>
>>>> bb->aux = NULL;
>>>> }
>>>> @@ -3012,16 +3006,13 @@ vect_bb_vectorization_profitable_p (bb_vec_info bb_vinfo)
>>>> static void
>>>> vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
>>>> {
>>>> - gimple_stmt_iterator gsi;
>>>> -
>>>> - for (gsi = bb_vinfo->region_begin;
>>>> - gsi_stmt (gsi) != gsi_stmt (bb_vinfo->region_end); gsi_next (&gsi))
>>>> + for (gimple *stmt : bb_vinfo->region_stmts)
>>>> {
>>>> - gassign *stmt = dyn_cast <gassign *> (gsi_stmt (gsi));
>>>> - if (!stmt || gimple_assign_rhs_code (stmt) != CONSTRUCTOR)
>>>> + gassign *assign = dyn_cast <gassign *> (stmt);
>>>> + if (!assign || gimple_assign_rhs_code (assign) != CONSTRUCTOR)
>>>> continue;
>>>>
>>>> - tree rhs = gimple_assign_rhs1 (stmt);
>>>> + tree rhs = gimple_assign_rhs1 (assign);
>>>> if (!VECTOR_TYPE_P (TREE_TYPE (rhs))
>>>> || maybe_ne (TYPE_VECTOR_SUBPARTS (TREE_TYPE (rhs)),
>>>> CONSTRUCTOR_NELTS (rhs))
>>>> @@ -3029,7 +3020,7 @@ vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
>>>> || uniform_vector_p (rhs))
>>>> continue;
>>>>
>>>> - stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
>>>> + stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (assign);
>>>> BB_VINFO_GROUPED_STORES (bb_vinfo).safe_push (stmt_info);
>>>> }
>>>> }
>>>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
>>>> index 6c830ad09f4..c94817e6af6 100644
>>>> --- a/gcc/tree-vectorizer.h
>>>> +++ b/gcc/tree-vectorizer.h
>>>> @@ -787,12 +787,92 @@ loop_vec_info_for_loop (class loop *loop)
>>>> typedef class _bb_vec_info : public vec_info
>>>> {
>>>> public:
>>>> + struct const_iterator
>>>> + {
>>>
>>> “{” should be directly under “struct”.
>>>
>>>> + const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
>>>> +
>>>> + const const_iterator &
>>>> + operator++ ()
>>>> + {
>>>
>>> “{” should be directly under “operator++”. Same for the other structs
>>> and functions.
>>>
>>>> + gsi_next (&gsi); return *this;
>>>> + }
>>>> +
>>>> + gimple *operator* () const { return gsi_stmt (gsi); }
>>>> +
>>>> + bool
>>>> + operator== (const const_iterator& other) const
>>>> + {
>>>> + return gsi_stmt (gsi) == gsi_stmt (other.gsi);
>>>> + }
>>>> +
>>>> + bool
>>>> + operator!= (const const_iterator& other) const
>>>> + {
>>>> + return !(*this == other);
>>>> + }
>>>> +
>>>> + gimple_stmt_iterator gsi;
>>>> + };
>>>> +
>>>> + struct reverse_iterator
>>>
>>> This should be a const_reverse_iterator.
>>>
>>>> + {
>>>> + reverse_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
>>>> +
>>>> + const reverse_iterator &
>>>> + operator++ ()
>>>> + {
>>>> + gsi_prev (&gsi); return *this;
>>>> + }
>>>> +
>>>> + gimple *operator* () const { return gsi_stmt (gsi); }
>>>> +
>>>> + bool
>>>> + operator== (const reverse_iterator& other) const
>>>> + {
>>>> + return gsi_stmt (gsi) == gsi_stmt (other.gsi);
>>>> + }
>>>> +
>>>> + bool
>>>> + operator!= (const reverse_iterator& other) const
>>>> + {
>>>> + return !(*this == other);
>>>> + }
>>>> +
>>>> + gimple_stmt_iterator gsi;
>>>> + };
>>>> +
>>>> + struct stmt_iterator
>>>> + {
>>>> + stmt_iterator (gimple_stmt_iterator region_begin,
>>>> + gimple_stmt_iterator region_end)
>>>> + : m_region_begin (region_begin), m_region_end (region_end) {}
>>>> +
>>>> + gimple_stmt_iterator region_begin () { return m_region_begin; }
>>>> +
>>>> + const_iterator begin () const { return const_iterator (m_region_begin); }
>>>> + const_iterator end () const { return const_iterator (m_region_end); }
>>>> +
>>>> + gimple_stmt_iterator m_region_begin;
>>>> + gimple_stmt_iterator m_region_end;
>>>> +
>>>> + reverse_iterator rbegin () const
>>>> + {
>>>> + reverse_iterator it = reverse_iterator (m_region_end);
>>>> + if (*it == NULL)
>>>> + return reverse_iterator (gsi_last_bb (m_region_end.bb));
>>>> + else
>>>> + return ++it;
>>>> + }
>>>> +
>>>> + reverse_iterator rend () const { return reverse_iterator (m_region_begin); }
>>>> + };
>>
>> Thank you for the review. I'm skipping for now the renaming and formatting changes which
>> I'll do later.
>>
>>>
>>> I think for this we should have a template class for begin/end iterator
>>> pairs, probably in coretypes.h. We could call it “iterator_pair” or
>>> something. Then “region_stmts” would return (see below) a:
>>>
>>> iterator_pair<const_iterator>
>>>
>>> and “reverse_region_stmts” would return:
>>>
>>> iterator_pair<const_reverse_iterator>
>>
>> Hmm, sounds like a nice abstraction but I see 2 problems with that:
>>
>> 1) How can I define a constructor of the iterator_pair when I need something like:
>> iterator_pair<const_iterator> (region_begin, region_end)?
>
> Not sure if I'm answering the right question, sorry, but I meant
> something along the lines of:
>
> template<typename T>
> struct iterator_pair
> {
> public:
> iterator_pair (const T &begin, const T &end)
> : m_begin (begin), m_end (end) {}
>
> T begin () const { return m_begin; }
> T end () const { return m_end; }
>
> private:
> T m_begin;
> T m_end;
> };
>
>> 2) rbegin function is more complex than begin function:
>>
>> reverse_iterator rbegin () const
>> {
>> reverse_iterator it = reverse_iterator (m_region_end);
>> if (*it == NULL)
>> return reverse_iterator (gsi_last_bb (m_region_end.bb));
>> else
>> return ++it;
>> }
>>
>> const_iterator begin () const { return const_iterator (m_region_begin); }
>>
>> How can we abstract that?
>
> With the change above, we'd replace “rbegin” and “rend”
> with a “reverse_region_stmts” function that returns an
> “iterator_pair<const_reverse_iterator>”. Its “begin” iterator
> would have the value that “rbegin” calculates above and its “end”
> iterator would have the same value as the current patch's “rend”.
All right, you provided a nice hints! There's updated version of the patch.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Thanks,
Martin
>
> Thanks,
> Richard
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vectorizer-add-_bb_vec_info-region_stmts-and-iterato.patch
Type: text/x-patch
Size: 7705 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200617/d3468b0b/attachment-0001.bin>
More information about the Gcc-patches
mailing list