Hi Kelvin,
On Wed, Jun 21, 2017 at 04:42:46PM -0600, Kelvin Nilsen wrote:
> This patch adds IEEE 128 support to the existing scalar_insert_exp,
> scalar_extract_exp, scalar_extract_sig, scalar_test_data_class, and
> scalar_test_neg rs6000 built-in functions. Test programs are provided
> to exercise the new IEEE 128 functionality and to validate forms of
> these built-in functions that do not depend on IEEE 128 support.
> * config/rs6000/rs6000-builtin.def (VSEEQP): Add scalar extract
Stray tab (after "scalar").
> +;; VSX Scalar Extract Exponent Quad-Precision
> +(define_insn "xsxexpqp"
> + [(set (match_operand:DI 0 "altivec_register_operand" "=v")
> + (unspec:DI [(match_operand:KF 1 "altivec_register_operand" "v")]
> + UNSPEC_VSX_SXEXPDP))]
> + "TARGET_P9_VECTOR && TARGET_64BIT"
> + "xsxexpqp %0,%1"
> + [(set_attr "type" "vecmove")])
TARGET_64BIT should probably be removed (and if not, a comment would help).
You also may want to explain the low half of the TI result is zeroed, but
we ignore it here? (Because some other insns use TI). Or maybe that is
obvious to people who actually know the instructions ;-)
> +;; VSX Scalar Extract Significand Quad-Precision
> +(define_insn "xsxsigqp"
> + [(set (match_operand:TI 0 "altivec_register_operand" "=v")
> + (unspec:TI [(match_operand:KF 1 "altivec_register_operand" "v")]
> + UNSPEC_VSX_SXSIGDP))]
> + "TARGET_P9_VECTOR && TARGET_64BIT"
> + "xsxsigqp %0,%1"
> + [(set_attr "type" "vecmove")])
Should this be UNSPEC_VSX_SXSIGQP? Or, if we can use the same unspec for
all data sizes, its name should not say "DP" :-)
(And TARGET_64BIT; please check it everywhere).
> +;; VSX Scalar Test Data Class Quad-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.)
> +(define_expand "xststdcqp"
> + [(set (match_dup 3)
> + (compare:CCFP
> + (unspec:KF
> + [(match_operand:KF 1 "vsx_register_operand" "v")
> + (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)))]
So this is specialised to only testing the "eq" part. That is fine, but
please add a comment?
> + operands[4] = CONST0_RTX (SImode);
Please write const0_rtx, instead, for scalar integer modes.
> +(define_insn "*xststdcqp"
> + [(set (match_operand:CCFP 0 "" "=y")
> + (compare:CCFP
> + (unspec:KF [(match_operand:KF 1 "altivec_register_operand" "v")
> + (match_operand:SI 2 "u7bit_cint_operand" "n")]
> + UNSPEC_VSX_STSTDC)
> + (match_operand:SI 3 "zero_constant" "j")))]
Can't you just write (const_int 0) here?
> + if (GET_MODE_PRECISION (arg1_mode) > 64)
> + {
> + /* if first argument is of float variety, choose variant
> + that expects __ieee128 argument. Otherwise, expect
> + __int128 argument. */
"If".
> + else
> + {
> + /* if first argument is of float variety, choose variant
(again).
> +/* { dg-do compile { target { powerpc*-*-* } } } */
Btw, tests in gcc.target/powerpc do not need this line: from powerpc.exp:
# Exit immediately if this isn't a PowerPC target.
if { ![istarget powerpc*-*-*] && ![istarget rs6000-*-*] } then {
return
}
(and compile is the default action).
Doesn't hurt either of course.
> The @code{scalar_extract_exp} and @code{scalar_extract_sig} built-in
> functions return the significand and the biased exponent value
> respectively of their @code{source} arguments.
> -Within the result returned by @code{scalar_extract_sig},
> -the @code{0x10000000000000} bit is set if the
> +When supplied with a 64-bit @code{source} argument, the
> +result returned by @code{scalar_extract_sig} has
> +the @code{0x10000000000000} bit set if the
> function's @code{source} argument is in normalized form.
> Otherwise, this bit is set to 0.
> +When supplied with a 128-bit @code{source} argument, the
> +@code{0x10000000000000000000000000000} bit of the result is
> +treated similarly.
Which bit is that? Hard to tell... Maybe write
@code{0x00010000000000000000000000000000} (and @code{0x0010000000000000})
to make it easier to read?
Please consider the trivialities above; the patch is okay for trunk.
Thanks,
Segher
