On Fri, May 13, 2016 at 07:25:43PM -0400, Michael Meissner wrote:
> This patch adds support for the 32-bit word splat instructions, the byte
> immediate splat instructions, and the vector sign extend instructions to GCC
> 7.0.
>
> In addition to the various splat instructions, since I was modifying the
> vector
> moves, I took the opportunity to reorganize the vector move instructions with
> several changes I've wanted to do:
It is much easier to review, and for regression searches later, if one
patch does one thing. No need to change this patch, but please keep
it in mind for later patches.
> I replaced the single move that handled all vector types with separate 32-bit
> and 64-bit moves. I also combined the movti_<bit> moves (when -mvsx-timode is
> in effect) into the main vector moves.
>
> I eliminated separate moves for <VSr> <VSa>, where the preferred register
> class
> (<VSr>) is listed first, and the secondary register class (<VSa>) is listed
> second with a '?' to discourage use.
>
> Prefer loading 0/-1 in any VSX register for ISA 3.0, and Altivec registers for
> ISA 2.06/2.07 (PR target/70915) so that if the register was involved in a slow
> operation, the clear/set operation does not wait for the slow operation to
> finish.
>
> I adjusted the length attributes for 32-bit mode.
>
> I changed the 32-bit move to use rs6000_output_move_128bit and drop the use of
> the string instructions for 32-bit movti when -mvsx-timode is in effect.
>
> I used spacing so that the alternatives and attributes don't generate long
> lines, and put things in columns, so that it is easier to match up the
> operands
> and attributes with the insn alternatives.
>
> I did a spec 2006 run comparing these changes to the trunk, and there were no
> significant differences in terms of performances.
>
> Are these patches ok to apply to the GCC 7.0 trunk?
Changelog is missing.
> +;; Generate the XXORC instruction which was added in ISA 2.07
> +(define_constraint "wM"
> + "vector constant with all 1's, ISA 2.07 and above"
> + (and (match_test "TARGET_P8_VECTOR")
> + (match_operand 0 "all_ones_constant")))
That's not a very good comment; a constraint doesn't generate anything.
"Used for", maybe?
> +(define_predicate "xxspltib_constant_split"
> + (match_code "const_vector,vec_duplicate,const_int")
> +{
> + int value = 256;
> + int num_insns = -1;
No tabs please, just a single space will do.
> + rtx cvt = ((TARGET_XSCVDPSPN)
> + ? gen_vsx_xscvdpspn_scalar (freg, sreg)
> + : gen_vsx_xscvdpsp_scalar (freg, sreg));
No parentheses around TARGET_XSCVDPSPN.
> +(define_insn "xxspltib_v16qi"
> + [(set (match_operand:V16QI 0 "vsx_register_operand" "=wa")
> + (vec_duplicate:V16QI (match_operand:SI 1 "s8bit_cint_operand" "i")))]
> + "TARGET_P9_VECTOR"
> {
> - return rs6000_output_move_128bit (operands);
> + operands[2] = GEN_INT (INTVAL (operands[1]) & 0xff);
> + return "xxspltib %x0,%2";
> }
Please use "n" instead of "i"? It shouldn't matter here, but at least it's
good documentation.
> +;; VSX store VSX load
> +;; VSX move direct move
> +;; direct move GPR store
> +;; GPR load GPR move
> +;; P9 const. AVX const.
> +;; P9 const. 0
> +;; -1 GPR const.
> +;; AVX store AVX load
"AVX"? Heh. AltiVec or VMX.
> +(define_insn "*vsx_mov<mode>_64bit"
> + [(set (match_operand:VSX_M 0 "nonimmediate_operand" "=wOZ, <VSa>,
> + <VSa>, *r,
> + *wo, $Y,
> + ??r, ??r,
> + wo, v,
> + v, ?<VSa>,
> + ?wh, ??r,
> + wZ, v")
This is more readable, excellent. Would four or so columns work even
better?
> +/* { dg-do compile { target { powerpc64le-*-* } } } */
Does the test not work on BE?
Okay for trunk, with a changelog and the nits fixed. Thanks,
Segher
