On Mon, Oct 20, 2014 at 10:10 PM, Carrot Wei <car...@google.com> wrote:
> Hi Richard
>
> An arm testcase that can reproduce this bug is attached.
>
> 2014-10-20 Guozhi Wei <car...@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.guent...@gmail.com> wrote:
>> On Fri, Oct 17, 2014 at 7:58 PM, Carrot Wei <car...@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.
>>
