Re: [PATCH] Fix up _mm_f{,n}m{add,sub}_s{s,d} (PR target/54564)

[Richard Henderson <rth at redhat dot com>]

On 09/13/2012 08:52 AM, Jakub Jelinek wrote:
> The following patch fixes it, by tweaking the header so that the first
> argument is not negated (we negate the second one instead), as we don't want
> to negate the high elements if e.g. for whatever reason combiner doesn't
> match it.  It fixes the expander to use a dup of the X operand as the high
> element provider for the pattern, removes the 231 alternatives (because
> those provide different destination high elements) and removes commutative
> marker (again, that would mean different high elements).

I don't think this is the best way to fix this up.

(1) Negating the second argument is arguably non-canonical rtl.

(2) It's not the best match if we were to extend these builtins to FMA4.
    There we really do have 4 inputs.  Thus

(define_insn "*fmai_fmadd_<mode>_4"
  [(set (match_operand:VF_128 0 "register_operand" "=x,x")
        (vec_merge:VF_128
          (fma:VF_128
            (match_operand:VF_128 1 "nonimmediate_operand" "%x,x")
            (match_operand:VF_128 2 "nonimmediate_operand" " x,m")
            (match_operand:VF_128 3 "nonimmediate_operand" "xm,x"))
          (match_operand:VF_128 4 "register_operand" "0,0")
          (const_int 1)))]
  "TARGET_FMA4"
  "vfmadd<ssescalarmodesuffix>\t{%3, %2, %1, %0|%0, %1, %2, %3}"
  [(set_attr "type" "ssemuladd")
   (set_attr "mode" "<MODE>")])

which we can just as easily do by passing the appropriate input to
the expander.

It would be nice if Intel cleaned up their documentation for the
builtin, explicitly saying which high bits to copy.  I agree that
the testcase is probably as normative as we'll get though.


r~