Re: [7/7] Pool alignment information for common bases

[Richard Sandiford <richard dot sandiford at linaro dot org>]

Richard Biener <richard.guenther@gmail.com> writes:
> On Mon, Jul 3, 2017 at 9:49 AM, Richard Sandiford
> <richard.sandiford@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.

I guess it should have been a separate patch though.

>>        || !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 well.  I'll do a separate pre-patch to C++-ify the structures.

Thanks,
Richard