Martin Liška <mli...@suse.cz> writes:
> On 6/12/20 3:22 PM, Richard Sandiford wrote:
>> Martin Liška <mli...@suse.cz> writes:
>>> diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
>>> index 636ad59c001..eac372e6abc 100644
>>> --- a/gcc/tree-vect-patterns.c
>>> +++ b/gcc/tree-vect-patterns.c
>>> @@ -5120,20 +5120,12 @@ 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 (gimple *stmt: *bb_vinfo)
>>> {
>>> - if (!gsi_stmt (si))
>>> - si = gsi_last_bb (bb_vinfo->bb);
>>> - else
>>> - gsi_prev (&si);
>>> - stmt = gsi_stmt (si);
>>> 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));
>>> }
>>> }
>>
>> This loop wants a reverse iterator: it starts at the end and walks
>> backwards. That's important because vect_determine_stmt_precisions
>> acts based on information recorded about later uses.
>
> I thought that it may be a problematic change. Note that we don't a have
> test coverage for the situation in testsuite (on x86_64). So I'm also
> introducing reverse_iterator for this.
There's definitely coverage on aarch64. Thought there would be on x86
too for the PAVG stuff, but obviously not.
>>> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
>>> index cdd6f6c5e5d..766598862d4 100644
>>> --- a/gcc/tree-vect-stmts.c
>>> +++ b/gcc/tree-vect-stmts.c
>>> @@ -1342,7 +1342,7 @@ vect_init_vector_1 (vec_info *vinfo, stmt_vec_info
>>> stmt_vinfo, gimple *new_stmt,
>>> else
>>> {
>>> bb_vec_info bb_vinfo = dyn_cast <bb_vec_info> (vinfo);
>>> - gimple_stmt_iterator gsi_region_begin = bb_vinfo->region_begin;
>>> + gimple_stmt_iterator gsi_region_begin = bb_vinfo->begin ().gsi;
>>> gsi_insert_before (&gsi_region_begin, new_stmt, GSI_SAME_STMT);
>>> }
>>> }
>>
>> Feels like this kind-of breaks the abstraction a bit.
>>
>> Would it make sense instead to add the operators to gimple_stmt_iterator
>> itself and just make const_iterator a typedef of that?
>
> Well, I'm planning to use the _bb_vec_info::const_iterator to jump in between
> BBs, so it can't be a simple typedef.
But what happens to this code when that happens? Is inserting at
begin().gsi meaningful? I.e. is the stmt at *begin() guaranteed
to dominate all the SLP code even with multple BBs?
Personally I'd prefer either:
(a) const_iterator starts out as a typedef of gimple_stmt_iterator,
and with your later changes becomes a derived class of it; or
(b) we just provide a region_begin() function that returns the gsi
directly.
> + 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;
> + }
I think the else case should return “++it” instead, since AIUI
m_region_end is an exclusive rather than inclusive endpoint.
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.
Thanks,
Richard
