This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, rs6000] gcc mainline, add builtin support for vec_float, vec_float2, vec_floate, vec_floate, builtins


Hi Carl,

A couple of issues, most small:

On Fri, Jun 09, 2017 at 11:20:43AM -0700, Carl E. Love wrote:
> +void
> +rs6000_generate_float2_code (bool signed_convert, rtx dst, rtx src1, rtx src2)
> +{
> +  rtx rtx_tmp0, rtx_tmp1, rtx_tmp2, rtx_tmp3;
> +
> +  rtx_tmp0 = gen_reg_rtx (V2DImode);
> +  rtx_tmp1 = gen_reg_rtx (V2DImode);
> +
> +  /* The vector merge instruction vmrgew swaps the 2nd and 3rd words,
> +     compensate by swapping the 64-bit elements around to negate the vmrgew
> +     swap. */

This comment isn't very clear to me...  Could you expand it a bit?

Oh, and dot space space.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -310,6 +310,10 @@
>  ;; Iterator for the 2 short vector types to do a splat from an integer
>  (define_mode_iterator VSX_SPLAT_I [V16QI V8HI])
>  
> +;; Mode iterator and attribute for vector floate and floato conversions
> +(define_mode_iterator VFC [V2DI V2DF])
> +(define_mode_attr VFC_inst [(V2DI "sxd") (V2DF "dp")])

.._sxddp, like VS_sxswp in altivec.md?  The iterator is just VSX_D.

Maybe some or all of these iterators/attrs should live in vector.md?

Is it really useful to have separate files altivec.md and vsx.md anymore?
Or should some things be moved?  This is a general question, not really
something to be handled in this patch ;-)

> +(define_insn "vsx_xvcvsxwsp"
> +  [(set (match_operand:V4SF 0 "vsx_register_operand" "=v")
> +	(unspec:V4SF[(match_operand:V4SI 1 "vsx_register_operand" "v")]
> +		    UNSPEC_VSX_CVSXWSP))]
> +  "VECTOR_UNIT_VSX_P (V4SFmode)"
> +  "xvcvsxwsp %x0,%x1"
> +  [(set_attr "type" "vecdouble")])

"v" is only the VRs...  Do you want "wa" or similar instead?

(Same question for everything, the expanders as well).

> +(define_expand "floate<mode>"
> +  [(match_operand:V4SF 0 "register_operand" "=v")
> +   (unspec:V4SF [(match_operand:VFC 1 "register_operand" "v")]
> +   UNSPEC_VSX_FLOATE)]
> +   "TARGET_VSX"
> +{
> +  if (VECTOR_ELT_ORDER_BIG)
> +    {
> +      /* Shift left one word to put even word correct location */
> +	rtx rtx_tmp;

This indent is incorrect.  Indent once after {, not twice.

> +DONE;
> +})

And an indent before DONE.

> +;; Generate uns_floate
> +;; convert long long unsigned to float
> +;;(Only even words are valid, BE numbering)

Space after ;; like on the previous lines.

> +  else
> +    {
> +      emit_insn (gen_vsx_xvcvuxdsp (operands[0], operands[1]));
> +    }

Don't make blocks for single statements, unless that really helps reading
(say, if there is a big comment before that statement).

Thanks,


Segher


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]