ptx preliminary address space fixes [3/4]

ptx preliminary address space fixes [3/4]

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]

The vectorizer can also replace a memory reference without ensuring it 
uses the correct address space.

Bootstrapped and tested together with the other patches on x86_64-linux. 
  Ok?


Bernd

[-- Attachment #2: ptx-as3.diff --]
[-- Type: text/x-patch, Size: 2244 bytes --]

commit e85dbde1aa3396b5e202aa736f96b232a6e11e86
Author: Bernd Schmidt <bernds@codesourcery.com>
Date:   Wed Sep 10 16:33:40 2014 +0200

    	* tree-vect-stmts.c (vectorizable_store, vectorizable_load): Apply
    	address spaces to the type of the MEM_REF as needed.

diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index 26eb2d4..5fca144 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -5026,6 +5026,9 @@ vectorizable_store (gimple stmt, gimple_stmt_iterator *gsi, gimple *vec_stmt,
       && TREE_CODE (scalar_dest) != MEM_REF)
     return false;
 
+  tree dest_type = TREE_TYPE (scalar_dest);
+  addr_space_t as = TYPE_ADDR_SPACE (dest_type);
+
   gcc_assert (gimple_assign_single_p (stmt));
   op = gimple_assign_rhs1 (stmt);
   if (!vect_is_simple_use (op, stmt, loop_vinfo, bb_vinfo, &def_stmt,
@@ -5038,6 +5041,11 @@ vectorizable_store (gimple stmt, gimple_stmt_iterator *gsi, gimple *vec_stmt,
     }
 
   elem_type = TREE_TYPE (vectype);
+  if (!ADDR_SPACE_GENERIC_P (as))
+    {
+      elem_type = apply_as_to_type (elem_type, as);
+      vectype = apply_as_to_type (vectype, as);
+    }
   vec_mode = TYPE_MODE (vectype);
 
   /* FORNOW. In some cases can vectorize even if data-type not supported
@@ -5379,7 +5387,7 @@ vectorizable_store (gimple stmt, gimple_stmt_iterator *gsi, gimple *vec_stmt,
 		   vect_permute_store_chain().  */
 		vec_oprnd = result_chain[i];
 
-	      data_ref = build2 (MEM_REF, TREE_TYPE (vec_oprnd), dataref_ptr,
+	      data_ref = build2 (MEM_REF, vectype, dataref_ptr,
 				 dataref_offset
 				 ? dataref_offset
 				 : build_int_cst (reference_alias_ptr_type
@@ -5692,8 +5700,17 @@ vectorizable_load (gimple stmt, gimple_stmt_iterator *gsi, gimple *vec_stmt,
   if (!STMT_VINFO_DATA_REF (stmt_info))
     return false;
 
+  tree rhs = gimple_assign_rhs1 (stmt);
+  tree rhstype = TREE_TYPE (rhs);
+  addr_space_t as = TYPE_ADDR_SPACE (rhstype);
+
   elem_type = TREE_TYPE (vectype);
   mode = TYPE_MODE (vectype);
+  if (!ADDR_SPACE_GENERIC_P (as))
+    {
+      elem_type = apply_as_to_type (elem_type, as);
+      vectype = apply_as_to_type (vectype, as);
+    }
 
   /* FORNOW. In some cases can vectorize even if data-type not supported
     (e.g. - data copies).  */

Re: ptx preliminary address space fixes [3/4]

[Richard Biener]

On Thu, Sep 11, 2014 at 12:12 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> The vectorizer can also replace a memory reference without ensuring it uses
> the correct address space.
>
> Bootstrapped and tested together with the other patches on x86_64-linux.
> Ok?

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);

+  if (!ADDR_SPACE_GENERIC_P (as))
+    {
+      elem_type = apply_as_to_type (elem_type, as);
+      vectype = apply_as_to_type (vectype, as);
+    }

but then I wonder why not simply build the correct vector types
in the first place in vect_analyze_data_refs?

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.

Thanks,
Richard.

>
> Bernd

Re: ptx preliminary address space fixes [3/4]

[Bernd Schmidt]

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.

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

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.

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


Bernd

Re: ptx preliminary address space fixes [3/4]

[Richard Biener]

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
>

Re: ptx preliminary address space fixes [3/4]

[Bernd Schmidt]

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

Re: ptx preliminary address space fixes [3/4]

[Richard Biener]

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
>

Re: ptx preliminary address space fixes [3/4]

[Bernd Schmidt]

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

Re: ptx preliminary address space fixes [3/4]

[Richard Biener]

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
>