Re: [PATCH, rs6000] Tidy implementation of vec_ldl

2018-03-30 Thread Segher Boessenkool 
Hi!

On Thu, Mar 29, 2018 at 09:47:42AM -0500, Kelvin Nilsen wrote:
> In summary, this patch removes two prototypes:
> 
>   vector int vec_ldl (int, long int *)
>   vector unsigned int vec_ldl (int, unsigned long int *)
> 
> and adds eight:
> 
>   vector bool char vec_ldl (int, bool char *)
>   vector bool short vec_ldl (int, bool short *)
>   vector bool int vec_ldl (int, bool int *)
>   vector bool long long vec_ldl (int, bool long long *)
>   vector pixel vec_ldl (int, pixel *)

Is "pixel" a type?  "vector pixel" is, but plain "pixel"?

>   vector long long vec_ldl (int, long long *)
>   vector unsigned long long vec_ldl (int, unsigned long long *)

>     * config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Remove
>     erroneous entries for
>     "vector int vec_ldl (int, long int *)", and
>     "vector    unsigned int vec_ldl (int, unsigned long int *)".

Whitespace is weird.  I'll just assume this is how you pasted it into
the mail and it will be correct in the changelog you commit.

> @@ -32855,7 +32855,7 @@ rs6000_mangle_type (const_tree type)
>    if (type == bool_short_type_node) return "U6__bools";
>    if (type == pixel_type_node) return "u7__pixel";
>    if (type == bool_int_type_node) return "U6__booli";
> -  if (type == bool_long_type_node) return "U6__booll";
> +  if (type == bool_long_long_type_node) return "U6__booll";

The mangled name now still says it is "bool long", not "bool long long".
Can you do this?  At the very least it needs a comment.

> --- gcc/config/rs6000/rs6000-c.c    (revision 258800)
> +++ gcc/config/rs6000/rs6000-c.c    (working copy)
> @@ -1656,29 +1656,50 @@ const struct altivec_builtin_types altivec_overloa
>  RS6000_BTI_V16QI, RS6000_BTI_INTSI, ~RS6000_BTI_INTQI, 0 },
>    { ALTIVEC_BUILTIN_VEC_LVEBX, ALTIVEC_BUILTIN_LVEBX,
>  RS6000_BTI_unsigned_V16QI, RS6000_BTI_INTSI, ~RS6000_BTI_UINTQI, 0 
> },
> +
> +  /* vector float vec_ldl (int, vector float *);
> +   * vector float vec_ldl (int, float *); */

No leading * on comment continuation lines.

> --- gcc/testsuite/gcc.target/powerpc/vec-ldl-1.c (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/vec-ldl-1.c    (revision 258889)
> @@ -0,0 +1,214 @@
> +/* { dg-do run { target powerpc*-*-* } } */
> +/* { dg-require-effective-target vmx_hw } */
> +/* { dg-require-effective-target lp64 } */
> +/* { dg-options "-maltivec -O0 -Wall" } */

Does this need lp64?  Same question for the other tests.


Segher

[PATCH, rs6000] Tidy implementation of vec_ldl

2018-03-29 Thread Kelvin Nilsen 

During code review, an inconsistency was noticed in some
of the prototypes defined for the vec_ldl built-in function.
In particular, the vector fetched from an address declare
to be long long * was returned as "vector int".  In
addressing this problem, certain other inconsistencies
and omissions were discovered.  This patch tidies up
the implementation of this function.  A separate patch
is in preparation to address the documentation for this
and all other PowerPC built-in functions.

In summary, this patch removes two prototypes:

  vector int vec_ldl (int, long int *)
  vector unsigned int vec_ldl (int, unsigned long int *)

and adds eight:

  vector bool char vec_ldl (int, bool char *)
  vector bool short vec_ldl (int, bool short *)
  vector bool int vec_ldl (int, bool int *)
  vector bool long long vec_ldl (int, bool long long *)
  vector pixel vec_ldl (int, pixel *)
  vector long long vec_ldl (int, long long *)
  vector unsigned long long vec_ldl (int, unsigned long long *)

This patch has been bootstrapped and tested without
regressions on both powerpc64le-unknown-linux (P8)
and on powerpc-linux (P7 big-endian, with both -m32
and -m64 target options).

Is this ok for trunk?

gcc/ChangeLog:

2018-03-29  Kelvin Nilsen 

    * config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Remove
    erroneous entries for
    "vector int vec_ldl (int, long int *)", and
    "vector    unsigned int vec_ldl (int, unsigned long int *)".
    Add comments and entries for
    "vector bool char vec_ldl (int, bool char *)",
    "vector bool short vec_ldl (int, bool short *)",
    "vector bool int vec_ldl (int, bool int *)",
    "vector bool long long vec_ldl (int, bool long long *)",
    "vector pixel vec_ldl (int, pixel *)",
    "vector long long vec_ldl (int, long long *)",
    "vector unsigned long long vec_ldl (int, unsigned long long *)".
    * config/rs6000/rs6000.c (rs6000_init_builtins): Initialize new
    type tree bool_long_long_type_node and correct definition of
    bool_V2DI_type_node to make reference to this new type tree.
    (rs6000_mangle_type): Replace erroneous reference to
    bool_long_type_node with bool_long_long_type_node.
    * config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Add
    comments to emphasize sign distinctions for char and int types and
    replace RS6000_BTI_bool_long constant with
    RS6000_BTI_bool_long_long constant.
    (bool_long_type_node): Remove this macro definition.
    (bool_long_long_type_node): New macro definition

gcc/testsuite/ChangeLog:

2018-03-29  Kelvin Nilsen 

    * gcc.target/powerpc/vec-ldl-1.c: New test.
    * gcc.dg/vmx/ops-long-1.c: Correct test programs to reflect
    corrections to ABI implementation.

Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c    (revision 258800)
+++ gcc/config/rs6000/rs6000.c    (working copy)
@@ -16947,7 +16947,7 @@ rs6000_init_builtins (void)
   bool_char_type_node = build_distinct_type_copy 
(unsigned_intQI_type_node);
   bool_short_type_node = build_distinct_type_copy 
(unsigned_intHI_type_node);
   bool_int_type_node = build_distinct_type_copy 
(unsigned_intSI_type_node);
-  bool_long_type_node = build_distinct_type_copy 
(unsigned_intDI_type_node);
+  bool_long_long_type_node = build_distinct_type_copy 
(unsigned_intDI_type_node);

   pixel_type_node = build_distinct_type_copy (unsigned_intHI_type_node);

   long_integer_type_internal_node = long_integer_type_node;
@@ -17064,7 +17064,7 @@ rs6000_init_builtins (void)
   bool_V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64
                     ? "__vector __bool long"
                     : "__vector __bool long long",
-                        bool_long_type_node, 2);
+                        bool_long_long_type_node, 2);
   pixel_V8HI_type_node = rs6000_vector_type ("__vector __pixel",
                  pixel_type_node, 8);

@@ -32855,7 +32855,7 @@ rs6000_mangle_type (const_tree type)
   if (type == bool_short_type_node) return "U6__bools";
   if (type == pixel_type_node) return "u7__pixel";
   if (type == bool_int_type_node) return "U6__booli";
-  if (type == bool_long_type_node) return "U6__booll";
+  if (type == bool_long_long_type_node) return "U6__booll";

   /* Use a unique name for __float128 rather than trying to use "e" or 
"g". Use
  "g" for IBM extended double, no matter whether it is long double 
(using

Index: gcc/config/rs6000/rs6000.h
===
--- gcc/config/rs6000/rs6000.h    (revision 258800)
+++ gcc/config/rs6000/rs6000.h    (working copy)
@@ -2578,7 +2578,7 @@ enum rs6000_builtin_type_index
   RS6000_BTI_opaque_V2SF,
   RS6000_BTI_opaque_p_V2SI,
   RS6000_BTI_opaque_V4SI,
-  RS6000_BTI_V16QI,
+  RS6000_BTI_V16QI,  /* __vector signed char */
   RS6000_BTI_V1TI,
   RS6000_BTI_V2SI,
   RS6000_BTI_V2SF,
@@ -2588,7 +2588,7 @@ enum rs6000_builtin_type_index
   RS6000_BTI_