Hi!
On Mon, Jun 01, 2020 at 09:15:00AM -0700, Carl Love wrote:
> The following patch adds support for the vec_splati, vec_splatid and
> vec_splati_ins builtins.
> * config/rs6000/altivec.h: Add define for vec_splati,
> vec_splatid
> and vec_splati_ins.
* config/rs6000/altivec.h (vec_splati, vec_splatid, vec_splati_ins):
New defines.
Etc.
> +(define_insn "vxxspltiw_v4si"
> + [(set (match_operand:V4SI 0 "register_operand" "=wa")
> + (unspec:V4SI [(match_operand:SI 1 "s32bit_cint_operand" "n")]
> + UNSPEC_XXSPLTIW))]
> + "TARGET_FUTURE"
> + "xxspltiw %x0,%1"
> + [(set_attr "type" "vecsimple")])
I think we can do a nicer name than "vxx"? The mode is part of the name
already, so that says it is vector? And the exact insn used is not
usually something you want in the name.
Maybe just "splat_imm_v4si" or similar? What do we do for the existing
immediate splats, hrm. ... Yeah, similar to what you do, so let's
just go with that for now... But "xx", not "vxx"?
> + /* Instruction uses destination as a source. Do not overwrite
> source. */
(Your patches line-wrapped in the mail, btw.)
> + emit_move_insn (operands[0], operands[1]);
> + emit_insn (gen_vxxsplti32dx_v4sf_inst (operands[0], GEN_INT( index
> ),
No spaces around "index" please. But there should be a space before the
opening parenthesis.
> +;; Return 1 if op is a 32-bit constant signed integer
> +(define_predicate "s32bit_cint_operand"
> + (and (match_code "const_int")
> + (match_test "INTVAL (op) >= -2147483648
> + && INTVAL (op) <= 2147483647")))
There probably is a nicer way to write this than with big decimal
numbers. (I'll not suggest one here because I'll just make a fool of
myself with overflow or signed/unsigned etc. :-) )
> +;; Return 1 if op is a constant 32-bit signed or unsigned integer
> +(define_predicate "c32bit_cint_operand"
> + (and (match_code "const_int")
> + (match_test "((INTVAL (op) >> 32) == 0)")))
This does not work for negative 32-bit numbers? In GCC the LHS
expression is -1 for those... Not sure what it is for the C++11 we now
require, but in C11 it is implementation-defined, so not good either.
> +;; Return 1 if op is a constant 32-bit floating point value
> +(define_predicate "f32bit_const_operand"
> + (match_code "const_double"))
Either the predicate name is misleading (if you do allow all
const_double values), or there should be some test for the alloed values
here.
> +extern long long rs6000_constF32toI32 (rtx operand);
Please use rs6000_const_f32_to_i32 or similar, or a more meaningful
name (neither "f32" nor "i32" means anything in GCC).
const_float_as_integer? Something like that?
> +long long
> +rs6000_constF32toI32 (rtx operand)
> +{
> + long long value;
> + const struct real_value *rv = CONST_DOUBLE_REAL_VALUE (operand);
> +
> + if (GET_MODE (operand) != SFmode)
> + {
> + printf("ERROR, rs6000_constF32toI32 mode not equal to
> SFmode.\n");
Let's not have the printf :-)
> + gcc_unreachable ();
Is the gcc_unreachable still useful? If so, write it as
gcc_assert (GET_MODE (operand) == SFmode);
? And if not, just drop it :-)
> +@smallexample
> +@exdent vector double vec_splatid (const float);
> +@end smallexample
> +
> +Convert a floating-point value to double-precision and splat the
> result to a
> +vector of double-precision floats.
You probably should say the floating-point value you start with is
single precision.
Segher
