This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Basic block SLP



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...

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.
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]