Re: PATCH: named address space support (1/2: target-independent parts)

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

On Wed, Aug 20, 2008 at 7:50 AM, Ben Elliston <bje@au1.ibm.com> wrote:
> This patch introduces all of the target-independent code from the
> named-addr-spaces-branch into mainline.  It implements a sensible subset
> of the named address space functionality described in:
>
>  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf
>
> The current state of the implementation is:
>
>  - a target can define any number of address spaces;
>
>  - the target defines how pointers are transformed when
>    cast to/from the generic address space;
>
>  - casts between two non-generic address spaces is not yet
>    implemented, but there is nothing about the implementation
>    that would prevent this from being added at a later
>    stage.
>
> I will post a follow-up patch that brings across the SPU backend
> changes.
>
> This code has been in use in the Cell SDK compiler for some time, so is
> quite well exercised already.  This patch has been tested with a
> bootstrap on x86_64-linux and powerpc-linux, regression tested on
> spu-elf (no regressions), and doco changes were tested by "make info
> dvi" and visual inspection.
>
> Okay for mainline?

There are several new functions in the patch without a comment.

  BOOL_BITFIELD saturating_p : 1;
+  /* Whether the declaration is in another address space.  */
+  unsigned char address_space;
 };

"Whether" looks incorrect.

@@ -2279,6 +2296,8 @@ struct tree_type GTY(())
  unsigned user_align : 1;

  unsigned int align;
+  unsigned char address_space;
+
  tree pointer_to;
  tree reference_to;
  union tree_type_symtab {

please re-order fields to not increase the size of tree_type too much.
Move the alias_set member next to the align member and add the
address_space member before align to allow packing with the
bitfields if we ever decide to allocate more of them.

@@ -146,6 +146,7 @@ typedef struct mem_attrs GTY(())
  rtx offset;                  /* Offset from start of DECL, as CONST_INT.  */
  rtx size;                    /* Size in bytes, as a CONST_INT.  */
  unsigned int align;          /* Alignment of MEM in bits.  */
+  unsigned char addrspace;     /* Address space (0 for generic).  */
 } mem_attrs;

likewise, please move the align member next to the alias_set member
while you are at it.

@@ -1211,7 +1217,9 @@ useless_type_conversion_p (tree outer_ty
     recursing though.  */
  if (POINTER_TYPE_P (inner_type)
      && POINTER_TYPE_P (outer_type)
-      && TREE_CODE (TREE_TYPE (outer_type)) == VOID_TYPE)
+      && TREE_CODE (TREE_TYPE (outer_type)) == VOID_TYPE
+      && GENERIC_ADDR_SPACE_POINTER_TYPE_P (inner_type)
+      && GENERIC_ADDR_SPACE_POINTER_TYPE_P (outer_type))
    return true;

in other places you simply replaced POINTER_TYPE_P with
GENERIC_ADDR_SPACE_POINTER_TYPE_P - why is this not
correct here?

As a general remark, instead of using 'unsigned char' for the
address-space number I would have prefered that you introduce
a typedef somewhere.

The middle-end parts look ok, though I think the majority of the
patch should be reviewed by a C frontend maintainer.  Out of
curiosity, does GNU C++ after this patch support named
address-spaces?

Thanks,
Richard.