On Tue, Jul 4, 2017 at 2:01 PM, Richard Sandiford
<richard.sandif...@linaro.org> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>> On Mon, Jul 3, 2017 at 9:49 AM, Richard Sandiford
>> <richard.sandif...@linaro.org> wrote:
>>> @@ -2070,8 +2143,7 @@ vect_find_same_alignment_drs (struct dat
>>> if (dra == drb)
>>> return;
>>>
>>> - if (!operand_equal_p (DR_BASE_OBJECT (dra), DR_BASE_OBJECT (drb),
>>> - OEP_ADDRESS_OF)
>>> + if (!operand_equal_p (DR_BASE_ADDRESS (dra), DR_BASE_ADDRESS (drb), 0)
>>
>> Why this change? It's semantically weaker after your change.
>
> It's because the DR_BASE_OBJECT comes from the access_fn analysis
> while the DR_BASE_ADDRESS comes from the innermost_loop_behavior.
> I hadn't realised when adding the original code how different the
> two were, and since all the other parts are based on the
> innermost_loop_behavior, I think this check should be too.
> E.g. it doesn't really make sense to compare DR_INITs based
> on DR_BASE_OBJECT.
Ah ok, makes sense now.
> I guess it should have been a separate patch though.
No need.
>>> || !operand_equal_p (DR_OFFSET (dra), DR_OFFSET (drb), 0)
>>> || !operand_equal_p (DR_STEP (dra), DR_STEP (drb), 0))
>>> return;
>>> @@ -2129,6 +2201,7 @@ vect_analyze_data_refs_alignment (loop_v
>>> vec<data_reference_p> datarefs = vinfo->datarefs;
>>> struct data_reference *dr;
>>>
>>> + vect_record_base_alignments (vinfo);
>>> FOR_EACH_VEC_ELT (datarefs, i, dr)
>>> {
>>> stmt_vec_info stmt_info = vinfo_for_stmt (DR_STMT (dr));
>>> @@ -3327,7 +3400,8 @@ vect_analyze_data_refs (vec_info *vinfo,
>>> {
>>> struct data_reference *newdr
>>> = create_data_ref (NULL, loop_containing_stmt (stmt),
>>> - DR_REF (dr), stmt, maybe_scatter ? false : true);
>>> + DR_REF (dr), stmt, !maybe_scatter,
>>> + DR_IS_CONDITIONAL_IN_STMT (dr));
>>> gcc_assert (newdr != NULL && DR_REF (newdr));
>>> if (DR_BASE_ADDRESS (newdr)
>>> && DR_OFFSET (newdr)
>>> Index: gcc/tree-vect-slp.c
>>> ===================================================================
>>> --- gcc/tree-vect-slp.c 2017-07-03 08:20:56.404763323 +0100
>>> +++ gcc/tree-vect-slp.c 2017-07-03 08:42:51.149380545 +0100
>>> @@ -2358,6 +2358,7 @@ new_bb_vec_info (gimple_stmt_iterator re
>>> gimple_stmt_iterator gsi;
>>>
>>> res = (bb_vec_info) xcalloc (1, sizeof (struct _bb_vec_info));
>>> + new (&res->base_alignments) vec_base_alignments ();
>>
>> Ick. I'd rather make this proper C++ and do
>>
>> res = new _bb_vec_info;
>>
>> and add a constructor to the vec_info base initializing the hashtable.
>> The above smells fishy.
>
> I knew I pushing my luck with that one. I just didn't want to have to
> convert the current xcalloc of loop_vec_info into a long list of explicit
> zero initializers. (OK, we have a lot of explicit zero assignments already,
> but certainly some fields rely on the calloc zeroing.)
>
>> Looks like vec<> are happy with .create () being called on a zeroed struct.
>>
>> Alternatively add .create () to hashtable/map.
>
> The difference is that vec<> is explicitly meant to be POD, whereas
> hash_map isn't (and I don't think we want it to be).
Ah, indeed. So your patch makes *vec_info no longer POD (no problem
but then use new/delete and constructors).
> Ah well. I'll do a separate pre-patch to C++-ify the structures.
Thanks.
Richard.
> Thanks,
> Richard
