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
