Re: [patch] Basic block SLP

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, May 21, 2009 at 1:55 PM, Ira Rosen <IRAR@il.ibm.com> wrote:
>
>
> Richard Guenther <richard.guenther@gmail.com> wrote on 20/05/2009 15:37:26:
>
>> On Wed, May 20, 2009 at 2:02 PM, Ira Rosen <IRAR@il.ibm.com> wrote:
>> >
>> >
>> > Richard Guenther <richard.guenther@gmail.com> wrote on 18/05/2009
> 12:39:44:
>> >
>> >> On Tue, May 5, 2009 at 12:23 PM, Ira Rosen <IRAR@il.ibm.com> wrote:
>> >>
>> >> Some comments follow.
>> >>
>> >> ?/* Bitmap of virtual variables to be renamed. ?*/
>> >> ?bitmap vect_memsyms_to_rename;
>> >> +bitmap vect_slp_memsyms_to_rename;
>> >>
>> >> the memsyms_to_rename bitmap should no longer be necessary
>> >> after the alias-improvements branch merge. ?Simply call
>> >> mark_sym_for_renaming (gimple_vop (cfun)); in place of
>> >> mark_set_for_renaming.
>> >
>> > Done.
>> > BTW, isn't this true for vect_memsyms_to_rename as well?
>>
>> Yes indeed. ?It would be nice if you'd remove it.
>
> Sure. I'll do that in a different patch.
>
>
> ...
>
>> >>
>> >> +/* Vectorization flags. ?*/
>> >> +#define VECTORIZATION_ENABLED (flag_tree_vectorize != 0)
>> >> +#define SLP_ENABLED (flag_slp_vectorize == 1)
>> >> +#define SLP_DISABLED (flag_slp_vectorize == 0)
>> >>
>> >> I don't like these too much - but hey...
>> >
>> > I added this because of a bit complicated logic here:
>> >
>> > static bool
>> > gate_vect_slp (void)
>> > {
>> > ?/* Apply SLP either if the vectorizer is on and the user didn't
> specify
>> > ? ? whether to run SLP or not, or if the SLP flag was set by the user.
> */
>> > ?return ((VECTORIZATION_ENABLED && !SLP_DISABLED) || SLP_ENABLED);
>> > }
>>
>> So SLP vectorization runs when -ftree-vectorize is on or when
>> -ftree-vectorize-slp is on but not when -fno-tree-vectorize-slp is
>> specified ...
>>
>> As this is the only place where the macros are used I would
>> like to see their expansion here, avoding their definition.
>
> OK, I will change that.
>
> ...
>
>> >> > 2. there are 23 tests that fail with: "error: alignment of array
>> > elements
>> >> > is greater than element size" (e.g.
>> > gcc.dg/torture/stackalign/nested-2.c
>> >> > and g++.dg/torture/stackalign/eh-alloca-1.C). They all have
>> >> > __attribute__((aligned(64))). The failure occurs when we are trying
> to
>> >> > vectorize the basic block and get a vector type for the aligned
> type. I
>> >> > guess, otherwise, we just never get to this check. Hence, these
>> > failures
>> >> > are not really related to SLP. What is the best way to handle this?
>> >>
>> >> You need to use the TYPE_MAIN_VARIANT for the element type
>> >> of the vector (though I don't see where arrays or SLP should come
>> >> into play in these testcases at all...).
>> >
>> > SLP checks every basic block, and if it finds data references, it
> checks if
>> > there is a vector type for the scalar type of the data reference. It
> calls
>> > make_vector_type() for a vector type, where array of this type is built
> for
>> > debug representation purposes (AFAIU).
>>
>> Doh indeed. ?The problem is the layout_type call there. ?As we
>> strip the single array type from the single FIELD_DECL of the
>> struct we build here in _all_ places I think a proper fix would
>> be to simply not build a structure here but directly set the
>> array type as TYPE_DEBUG_REPRESENTATION_TYPE
>> (which doesn't need layout_type).
>
> It is in layout_type call, but not in the call from make_vector_type, but
> from build_array_type:
>
> Breakpoint 1, layout_type (type=0x2b2860eb2240)
> at ../../gcc/gcc/stor-layout.c:1848
> 1848 ? ? ? ? ? ? ?error ("alignment of array elements is greater than
> element size");
> (gdb) back
> #0 ?layout_type (type=0x2b2860eb2240) at ../../gcc/gcc/stor-layout.c:1848
> #1 ?0x00000000008dc33c in type_hash_lookup (hashcode=2524125531, type=0x40)
> at ../../gcc/gcc/tree.c:4721
> #2 ?0x00000000008dc3c9 in type_hash_canon (hashcode=2524125531, type=0x40)
> at ../../gcc/gcc/tree.c:4772
> #3 ?0x00000000008dd1d1 in build_array_type (elt_type=0x2b2860e52600,
> index_type=0x2b2860dd90c0) at ../../gcc/gcc/tree.c:5851
> #4 ?0x00000000008f4d1d in make_vector_type (innertype=0x2b2860e52600,
> nunits=4, mode=VOIDmode) at ../../gcc/gcc/tree.c:7441
> #5 ?0x000000000089d9c8 in get_vectype_for_scalar_type
> (scalar_type=0x2b2860e52600) at ../../gcc/gcc/tree-vect-stmts.c:4348
> #6 ?0x0000000000bbc3ef in vect_analyze_data_refs (loop_vinfo=<value
> optimized out>, bb_vinfo=<value optimized out>)
> ? ?at ../../gcc/gcc/tree-vect-data-refs.c:2050
> ...
> (gdb) p debug_generic_expr (type)
> aligned[4]
> $6 = void
>
> So, the problem is in building the array type...

Hmm, ok.  I suggest you ignore these failures for now and file a bugreport.
I will take care of it.

Thanks,
Richard.

> Thanks,
> Ira
>
>>
>> Simply s/TREE_TYPE (TYPE_FIELDS (TYPE_DEBUG_REPRESENTATION_TYPE
>> (type)))/TYPE_DEBUG_REPRESENTATION_TYPE (type)/g
>> on all uses should make this work. ?Can you check this?
>>
>> The patch is ok with these changes. ?You can submit the
>> TYPE_DEBUG_REPRESENTATION_TYPE stuff separately,
>> it needs C++ FE approval and having it separate is probably
>> better anyway. ?(Or tell me if I should take care of it).
>>
>> Thanks,
>> Richard.
>>
>
>