Re: [PATCH PR63530] Fix the pointer alignment in vectorization

[Carrot Wei <carrot at google dot com>]

Thanks for the review.
Following patch has been committed. I will port them to 4.9 branch
several days later.

2014-10-22  Guozhi Wei  <carrot@google.com>

        PR tree-optimization/63530
        tree-vect-data-refs.c (vect_create_addr_base_for_vector_ref): Set
        pointer alignment according to DR_MISALIGNMENT.

2014-10-22  Guozhi Wei  <carrot@google.com>

        PR tree-optimization/63530
        gcc.dg/vect/pr63530.c: New testcase.



On Tue, Oct 21, 2014 at 1:04 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Oct 20, 2014 at 10:10 PM, Carrot Wei <carrot@google.com> wrote:
>> Hi Richard
>>
>> An arm testcase that can reproduce this bug is attached.
>>
>> 2014-10-20  Guozhi Wei  <carrot@google.com>
>>
>>         PR tree-optimization/63530
>>         gcc.target/arm/pr63530.c: New testcase.
>>
>>
>> Index: pr63530.c
>> ===================================================================
>> --- pr63530.c (revision 0)
>> +++ pr63530.c (revision 0)
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_neon } */
>> +/* { dg-options "-march=armv7-a -mfloat-abi=hard -mfpu=neon -marm -O2
>> -ftree-vectorize -funroll-loops --param
>> \"max-completely-peeled-insns=400\"" } */
>> +
>> +typedef struct {
>> +  unsigned char map[256];
>> +  int i;
>> +} A, *AP;
>> +
>> +void* calloc(int, int);
>> +
>> +AP foo (int n)
>> +{
>> +  AP b = (AP)calloc (1, sizeof (A));
>> +  int i;
>> +  for (i = n; i < 256; i++)
>> +    b->map[i] = i;
>> +  return b;
>> +}
>> +
>> +/* { dg-final { scan-assembler-not "vst1.64" } } */
>
> Can you make it a runtime testcase that fails?  This way it would be
> less target specific.
>
>> On Mon, Oct 20, 2014 at 1:19 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <carrot@google.com> wrote:
>>>
>>> I miss a testcase.  I also miss a comment before this code explaining
>>> why DR_MISALIGNMENT if not -1 is valid and why it is not valid if
>>
>> DR_MISALIGNMENT (dr) == -1 means some unknown misalignment, otherwise
>> it means some known misalignment.
>> See the usage in file tree-vect-stmts.c.
>
> I know that.
>
>>> 'offset' is supplied (what about 'byte_offset' btw?).  Also if peeling
>>
>> It is for conservative, so it doesn't change the logic when offset is supplied.
>> I've checked that most of the passed in offset are caused by negative
>> step, its impact to DR_MISALIGNMENT should have already be considered
>> in function vect_update_misalignment_for_peel, but the comments of
>> vect_create_addr_base_for_vector_ref does not guarantee this usage of
>> offset.
>>
>> The usage of byte_offset is quite broken, many direct or indirect
>> callers don't provide the parameters. So only the author can comment
>> this.
>
> Well - please make it consistent at least, (offset || byte_offset).
>
>>> for alignment aligned this ref (misalign == 0) you don't set the alignment.
>>>
>> I assume if no misalignment is specified, the natural alignment of the
>> vector type is used, and caused the wrong code in our case, is it
>> right?
>
> No, DR_MISALIGNMENT == 0 means "aligned".
>
> OTOH it's quite unnecessary to do all the dance with the alignment
> part of the SSA name info (unnecessary for the actual memory references
> created by the vectorizer).  The type of the access ultimatively provides
> the larger alignment - the SSA name info only may enlarge it even
> further (thus it's dangerous to specify larger than valid there).
>
> So if you don't want to make it really optimal wrt offset/byte_offset please
> do
>
>    if (offset || byte_offset || misalign == -1)
>     mark_ptr_info_alignment_unknown (...)
>    else
>     set_ptr_info_alignment (..., align, misalign);
>
> The patch is ok with this change and the testcase turned into a runtime one
> and moved to gcc.dg/vect/
>
> Thanks,
> Richard.
>
>>> Thus you may fix a bug (not sure without a testcase) but the new code
>>> certainly doesn't look 100% correct.
>>>
>>> That said, I would have expected that we can unconditionally do
>>>
>>>  set_ptr_info_alignment (..., align, misalign)
>>>
>>> if misalign is != -1 and if we adjust misalign by offset * step + byte_offset
>>> (usually both are constants).
>>>
>>> Also we can still trust the alignment copied from addr_base modulo
>>> vector element size even if DR_MISALIGN is -1.  This may matter
>>> for targets that require element-alignment for vector accesses.
>>>

Attachment: patch
Description: Binary data

Attachment: patch2
Description: Binary data