[PATCH,rs6000] Add built-in function support Power9 binary floating point operations

[Segher Boessenkool]

Hi Kelvin,

On Fri, Jul 29, 2016 at 09:47:55AM -0600, Kelvin Nilsen wrote:
> This patch adds built-in support for the following fourteen new binary
> floating point instructions introduced with the Power9 architecture: 

Some comments, mostly about whitespace:

> --- gcc/doc/extend.texi	(revision 238014)
> +++ gcc/doc/extend.texi	(working copy)

> +int scalar_test_data_class (float source, unsigned int condition);
> +int scalar_test_data_class (double source, unsigned int condition);
> +
> +int scalar_test_neg (float source)
> +int scalar_test_neg (double source)

These last two probably want a semicolon as well?

> +The @code{scalar_extract_sig} and @code{scalar_insert_exp}
> +functions require a 64-bit environment supporting ISA 3.0 or later.
> +The @code{scalar_extract_exp} and @code{vec_extract_sig} built-in
> +functions return the significand and exponent respectively of their
> +@code{source} arguments.  The 

Trailing space.

> +The @code{scalar_cmp_exp_gt}, @code{scalar_cmp_exp_lt},
> +@code{scalar_cmp_exp_eq}, and @code{scalar_cmp_exp_unordered} built-in
> +functions return a non-zero value if @code{arg1} is greater than, less
> +than, equal to, or not comparable to @code{arg2} respectively.  The
> +arguments are not comparable if one or the other equals NaN (not a
> +number). 

Trailing space.

> +__vector float
> +vec_insert_exp (__vector unsigned int significands,  __vector unsigned int exponents);
> +__vector double
> +vec_insert_exp (__vector unsigned long long int significands,
> +                __vector unsigned long long int exponents);

Break up the first of these to two lines as well?

> +/* { dg-skip-if "" { powerpc*-*-aix* } } */

I think you can do this in bfp.exp, for all tests at the same time?
Or will bfp/ include tests that can run on AIX, eventually?

> +/* This test should succeed only 64-bit configuration.  */

Maybe "only on 64-bit configurations"?

> +;; Iterator for scalar floating point types
> +(define_mode_iterator VSX_SF [DF SF])

There is an iterator SFDF already; is there a reason to use a new one?

> +(define_mode_attr vsx_sf_suffix [(DF "dp") (SF "sp")])

That is Fvsx already.

> +(define_mode_attr vsx_f_suffix [(V4SF "dp") (V2DF "sp")])

Those seem swapped?

You can make vsx_sf_suffix (or Fvsx) do these types as well.

> +(define_mode_attr VSX_F_INTEGER [(V4SF "V4SI") (V2DF "V2DI")])

VSi does this, too.

> +;; ISA 3.0 Binary Floating-Point Support
> +
> +;; VSX Scalar Extract Exponent Double-Precision
> +(define_insn "xsxexpdp"
> +  [(set (match_operand:DI 0 "register_operand" "=r")
> +  	(unspec:DI [(match_operand:DF 1 "vsx_register_operand" "wa")] 
> +        UNSPEC_VSX_SXEXPDP))]

These last two lines aren't indented correctly (no spaces before tabs,
and no spaces where you could use a tab).  Many variations below.
"contrib/check_GNU_style.sh" might help a bit.

> +  "xsxexpdp %0,%x1"
> +  [(set_attr "type" "fp")])

I think this should be "fpsimple"?

> +(define_insn "*xscmpexpdp"
> +  [(set (match_operand:CCFP 0 "" "=y")

No predicate?  cc_reg_operand?

> +;; VSX Scalar Test Data Class Double- and Single-Precision
> +;;  (The lt bit is set if operand 1 is negative.  The eq bit is set 
> +;;   if any of the conditions tested by operand 2 are satisfied.  
> +;;   The gt and unordered bits are cleared to zero.)

Trailing spaces.  There are more.

> +(define_expand "xststdc<vsx_sf_suffix>"
> +  [(set (match_dup 3)
> +        (compare:CCFP
> +         (unspec:VSX_SF
> +          [(match_operand:VSX_SF 1 "vsx_register_operand" "wa")
> +           (match_operand:SI 2 "u7bit_cint_operand" "n")]
> +          UNSPEC_VSX_STSTDC)
> +        (match_dup 4)))
> +   (set (match_operand:SI 0 "register_operand" "=r")
> +        (eq:SI (match_dup 3)
> +               (const_int 0)))
> +  ]

Don't put this ] on a separate line please.

> +(define_insn "*xststdc<vsx_sf_suffix>"
> +  [(set (match_operand:CCFP 0 "" "=y")
> +        (compare:CCFP
> +         (unspec:VSX_SF [(match_operand:VSX_SF 1 "vsx_register_operand" "wa")
> +                        (match_operand:SI 2 "u7bit_cint_operand" "n")]

The ( should line up here.  And, tabs.

> +;; VSX Vector Test Data Class Double and Single Precision
> +;;  The corresponding elements of the result vector are all ones
> +;;   if any of the conditions tested by operand 3 are satisfied.  

Something went wrong with the leading spaces here :-)

> @@ -4721,6 +4799,7 @@ altivec_resolve_overloaded_builtin (location_t loc
>    tree types[3], args[3];
>    const struct altivec_builtin_types *desc;
>    unsigned int n;
> +  bool unsupported_builtin;
>  
>    if (!rs6000_overloaded_builtin_p (fcode))
>      return NULL_TREE;
> @@ -5480,6 +5559,7 @@ assignment for unaligned loads and stores");
>        return build_int_cst (NULL_TREE, TYPE_VECTOR_SUBPARTS (types[0]));
>      }
>  
> +  unsupported_builtin = false;

Declare it here, instead of 750 lines above?

> +BU_P9V_VSX_2 (VSCEDPGT,	"scalar_cmp_exp_dp_gt", CONST, xscmpexpdp_gt)
> +BU_P9V_VSX_2 (VSCEDPLT,	"scalar_cmp_exp_dp_lt", CONST, xscmpexpdp_lt)
> +BU_P9V_VSX_2 (VSCEDPEQ,	"scalar_cmp_exp_dp_eq", CONST, xscmpexpdp_eq)
> +BU_P9V_VSX_2 (VSCEDPUO,	"scalar_cmp_exp_dp_unordered", CONST, xscmpexpdp_unordered)

You're using spaces instead of tabs for the last few parameters here.
Everything else uses tabs, it's best to follow style I think.

> --- gcc/config/rs6000/predicates.md	(revision 238014)
> +++ gcc/config/rs6000/predicates.md	(working copy)
> @@ -147,6 +147,11 @@
>    (and (match_code "const_int")
>         (match_test "INTVAL (op) >= 0 && INTVAL (op) <= 63")))
>  
> +;; Return 1 if op is an unsigned 7-bit constant integer.
> +(define_predicate "u7bit_cint_operand"
> +  (and (match_code "const_int")
> +       (match_test "INTVAL (op) >= 0 && UINTVAL (op) <= 127")))

Why UINTVAL for the latter?  IN_RANGE is a bit nicer as well I think.


Segher