Hi Carl,

On Fri, Jun 16, 2017 at 02:19:05PM -0700, Carl Love wrote:
>         * config/rs6000/rs6000-c.c (altivec_overloaded_builtins): Add

Indent is broken on this line.

>       ALTIVEC_BUILTIN_VMULESW, ALTIVEC_BUILTIN_VMULEUW,
>       ALTIVEC_BUILTIN_VMULOSW, ALTIVEC_BUILTIN_VMULOUW enties.

Typo ("entries").

>       * config/rs6000/rs6000.c (rs6000_gimple_fold_builtin(),
>       builtin_function_type()): Add needed ALTIVEC_BUILTIN_* case
>       statements.

No () please, just the names.

>       * config/rs6000/altivec.md (define_c_enum "unspec",
>       define_expand "vec_widen_umult_even_v4si",
>       define_expand "vec_widen_smult_even_v4si",
>       define_expand "vec_widen_umult_odd_v4si",
>       define_expand "vec_widen_smult_odd_v4si",
>       define_insn "altivec_vmuleuw", define_insn "altivec_vmulesw",
>       define_insn "altivec_vmulouw",  define_insn "altivec_vmulosw"): Add
>       support to generate vmuleuw, vmulesw, vmulouw, vmulosw instructions.

        (UNSPEC_VMULEUW, UNSPEC_VMULESW, UNSPEC_VMULOUW, UNSPEC_VMULOSW):
        New enum "unspec" values.
        (vec_widen_umult_even_v4si, vec_widen_smult_even_v4si,
        vec_widen_umult_odd_v4si, vec_widen_smult_odd_v4si, altivec_vmuleuw,
        altivec_vmulesw, altivec_vmulouw, altivec_vmulosw): New patterns.

(Or similar.  Mention all new names added.  Usually for new things, just
saying "New." or "New frobnitz." is enough; the changelog does not describe
the design, or why you added something: it says just what changed).

> +(define_expand "vec_widen_umult_even_v4si"
> +  [(use (match_operand:V2DI 0 "register_operand" ""))
> +   (use (match_operand:V4SI 1 "register_operand" ""))
> +   (use (match_operand:V4SI 2 "register_operand" ""))]

You can leave off the default (empty) constraint strings, in expanders.

> +  "TARGET_ALTIVEC"
> +{
> +  if (VECTOR_ELT_ORDER_BIG)
> +    emit_insn (gen_altivec_vmuleuw (operands[0], operands[1],
> operands[2]));

Your patch was wrapped in the mail.  Please try to prevent that, so that
other people can apply the patch to test out, etc.

> +(define_insn "altivec_vmuleuw"
> +  [(set (match_operand:V2DI 0 "register_operand" "=v")
> +        (unspec:V2DI [(match_operand:V4SI 1 "register_operand" "v")
> +                      (match_operand:V4SI 2 "register_operand" "v")]
> +                  UNSPEC_VMULEUW))]

The one-but-last line should be indented with tabs instead of spaces, too.
(This happens a few time more later in the patch; please check it all).

Okay for trunk with those last nits fixed.  Thanks!


Segher

Reply via email to