| Message ID | 5412D45A.5080705@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 12, 2014 at 1:09 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 09/11/2014 01:39 PM, Richard Biener wrote: > >> Seeing this it would be nice to abstract away the exact place we store >> the address-space in a memory reference. So - can you add a helper >> reference_addr_space () please? Thus do >> >> addr_space_t as = reference_addr_space (scalar_dest); > > > Ok, no problem. Thanks. >> but then I wonder why not simply build the correct vector types >> in the first place in vect_analyze_data_refs? > > > We do, kind of. But there are two problems, the first one addressed here in > the patch: > - data_ref = build2 (MEM_REF, TREE_TYPE (vec_oprnd), > dataref_ptr, > + data_ref = build2 (MEM_REF, vectype, dataref_ptr, > > We use the wrong vector type (vectype has the right address space, but > vec_oprnd is an SSA_NAME). Ah, ok. That part is fine then. > The second problem is our reprentation of vector types. When given > <address-space-5> unsigned int > make_vector_type makes > <address-space-5> vector(2) unsigned int > rather than > <address-space-5> vector(2) <address-space-5> unsigned int > > Since we use elem_type in a few ways in tree_vectorizable_stmt, that looks > like it would cause problems, but it probably can be addressed in a simpler > way than what I originally had: > > Index: gomp-4_0-branch/gcc/tree-vect-stmts.c > =================================================================== > --- gomp-4_0-branch.orig/gcc/tree-vect-stmts.c > +++ gomp-4_0-branch/gcc/tree-vect-stmts.c > @@ -5037,7 +5037,8 @@ vectorizable_store (gimple stmt, gimple_ > return false; > } > > - elem_type = TREE_TYPE (vectype); > + elem_type = apply_addr_space_to_type (TREE_TYPE (vectype), > + TYPE_ADDR_SPACE (vectype)); > vec_mode = TYPE_MODE (vectype); > > /* FORNOW. In some cases can vectorize even if data-type not supported > @@ -5692,7 +5693,8 @@ vectorizable_load (gimple stmt, gimple_s > if (!STMT_VINFO_DATA_REF (stmt_info)) > return false; > > - elem_type = TREE_TYPE (vectype); > + elem_type = apply_addr_space_to_type (TREE_TYPE (vectype), > + TYPE_ADDR_SPACE (vectype)); > mode = TYPE_MODE (vectype); > > /* FORNOW. In some cases can vectorize even if data-type not supported > > The other alternative is to fix up make_vector_type. That could look like > this: > > Index: gomp-4_0-branch/gcc/tree.c > =================================================================== > --- gomp-4_0-branch.orig/gcc/tree.c > +++ gomp-4_0-branch/gcc/tree.c > @@ -9470,9 +9470,13 @@ make_vector_type (tree innertype, int nu > inner type. Use it to build the variant we return. */ > if ((TYPE_ATTRIBUTES (innertype) || TYPE_QUALS (innertype)) > && TREE_TYPE (t) != innertype) > - return build_type_attribute_qual_variant (t, > - TYPE_ATTRIBUTES (innertype), > - TYPE_QUALS (innertype)); > + { > + t = build_type_attribute_qual_variant (t, > + TYPE_ATTRIBUTES (innertype), > + TYPE_QUALS (innertype)); > + TREE_TYPE (t) = apply_addr_space_to_type (TREE_TYPE (t), > + TYPE_ADDR_SPACE > (innertype)); > + } > > return t; > } > > Let me know what you prefer. Hmm, neither I suppose. COMPLEX_TYPEs are also built with main-variant component type and I suspect the same for ARRAY_TYPEs. I see the address-space on types as artifact that comes from Frontend support (aka parsing). >> Or apply the addr-space to the memory reference with a new helper >> reference_apply_addr_space >> >> - data_ref = build2 (MEM_REF, TREE_TYPE (vec_oprnd), >> dataref_ptr, >> dataref_offset >> ? dataref_offset >> : build_int_cst >> (reference_alias_ptr_type >> .. >> reference_apply_addr_space (data_ref, as); >> >> at least that's how it's abstracted on the RTL side. I think I'd prefer >> if things would be working that way on the tree level, too. > > > I'm adapting my other patches to use such a function, but in this particular > case I think it would be best to fix up the types in advance since we build > accesses in several places and especially vectorizable_load also seems to > create operations on the pointer type. Are you ok with that? Fixing up the vector type in advance is ok with me but I'd like us to move away from address-space-on-types. Thanks, Richard. > > Bernd >
On 09/12/2014 01:45 PM, Richard Biener wrote: >> Let me know what you prefer. > > Hmm, neither I suppose. COMPLEX_TYPEs are also built > with main-variant component type and I suspect the same for > ARRAY_TYPEs. I see the address-space on types as > artifact that comes from Frontend support (aka parsing). > Fixing up the vector type in advance is ok with me but I'd like us to > move away from address-space-on-types. Is that an approval for the first variant in the sense that it's the best we can do at the moment? Or are you requiring a rewrite of all the address space support in the compiler? Bernd
On 09/12/2014 01:45 PM, Richard Biener wrote: > Fixing up the vector type in advance is ok with me but I'd like us to > move away from address-space-on-types. After thinking about it for a while, this idea makes no sense. Address spaces must be represented in the type system somehow - consider a pointer to an object in address space 0 vs. a pointer to an object in address space 1. These are different types, they may even have different sizes. So by adding address spaces to references (_DECLs and _REFs) the only thing we'd accomplish is duplicating existing information, with enhanced chances of getting inconsistencies. Bernd
On Fri, Sep 12, 2014 at 1:51 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 09/12/2014 01:45 PM, Richard Biener wrote: >>> >>> Let me know what you prefer. >> >> >> Hmm, neither I suppose. COMPLEX_TYPEs are also built >> with main-variant component type and I suspect the same for >> ARRAY_TYPEs. I see the address-space on types as >> artifact that comes from Frontend support (aka parsing). > > >> Fixing up the vector type in advance is ok with me but I'd like us to >> move away from address-space-on-types. > > > Is that an approval for the first variant in the sense that it's the best we > can do at the moment? Or are you requiring a rewrite of all the address > space support in the compiler? The former. Of course if you want to spend the time rewriting the GIMPLE parts of address-space support even better (shouldn't be too hard given nobody really cares about it too much). I just think that by modeling an API that looks like we have "fixed" the GIMPLE parts makes it easier for somebody to do that. Thanks, Richard. > > Bernd >
On Fri, Sep 12, 2014 at 2:14 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 09/12/2014 01:45 PM, Richard Biener wrote: >> >> Fixing up the vector type in advance is ok with me but I'd like us to >> move away from address-space-on-types. > > > After thinking about it for a while, this idea makes no sense. Address > spaces must be represented in the type system somehow - consider a pointer > to an object in address space 0 vs. a pointer to an object in address space > 1. These are different types, they may even have different sizes. > > So by adding address spaces to references (_DECLs and _REFs) the only thing > we'd accomplish is duplicating existing information, with enhanced chances > of getting inconsistencies. Well. There are two parts of adress-space suport. First is pointer types which may have different size/mode. Second is memory references to different address-spaces which may require different insns in the end (RTL). On RTL we get away with "lowering" pointers properly (the size/precision should be encoded correctly on the tree/GIMPLE level as well). And on RTL we have the address-space of a MEM in its MEM_ATTRs. On GIMPLE we weirdly use some TYPE_QUALS on some type contained in a memory reference tree. I'd like to fix the latter by placing address-space info on the reference itself (like on RTL), not on the types. Conveniently that would be on the base object we access which is either a DECL or a MEM_REF/TARGET_MEM_REF. Yes, the _frontends_ would be required to properly build memory references to objects in different address-spaces. Richard. > > Bernd >
Index: gomp-4_0-branch/gcc/tree-vect-stmts.c =================================================================== --- gomp-4_0-branch.orig/gcc/tree-vect-stmts.c +++ gomp-4_0-branch/gcc/tree-vect-stmts.c @@ -5037,7 +5037,8 @@ vectorizable_store (gimple stmt, gimple_ return false; } - elem_type = TREE_TYPE (vectype); + elem_type = apply_addr_space_to_type (TREE_TYPE (vectype), + TYPE_ADDR_SPACE (vectype)); vec_mode = TYPE_MODE (vectype); /* FORNOW. In some cases can vectorize even if data-type not supported @@ -5692,7 +5693,8 @@ vectorizable_load (gimple stmt, gimple_s if (!STMT_VINFO_DATA_REF (stmt_info)) return false; - elem_type = TREE_TYPE (vectype); + elem_type = apply_addr_space_to_type (TREE_TYPE (vectype), + TYPE_ADDR_SPACE (vectype)); mode = TYPE_MODE (vectype); /* FORNOW. In some cases can vectorize even if data-type not supported The other alternative is to fix up make_vector_type. That could look like this: Index: gomp-4_0-branch/gcc/tree.c =================================================================== --- gomp-4_0-branch.orig/gcc/tree.c +++ gomp-4_0-branch/gcc/tree.c @@ -9470,9 +9470,13 @@ make_vector_type (tree innertype, int nu inner type. Use it to build the variant we return. */ if ((TYPE_ATTRIBUTES (innertype) || TYPE_QUALS (innertype)) && TREE_TYPE (t) != innertype) - return build_type_attribute_qual_variant (t, - TYPE_ATTRIBUTES (innertype), - TYPE_QUALS (innertype)); + { + t = build_type_attribute_qual_variant (t, + TYPE_ATTRIBUTES (innertype), + TYPE_QUALS (innertype)); + TREE_TYPE (t) = apply_addr_space_to_type (TREE_TYPE (t), + TYPE_ADDR_SPACE (innertype)); + } return t; }