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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Sep 13, 2012 at 10:42 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Sep 13, 2012 at 5:52 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>> The fma-*.c testcase show that these intrinsics probably mean to preserve
>> the high elements (other than the lowest) of the first argument of the
>> fmaintrin.h *_s{s,d} intrinsics in the destination (the HW insn preserve
>> there the destination register, but that varies - for 132 and 213 it is the
>> first one (but the negation performed for _mm_fnm*_s[sd] breaks it anyway),
>> for 231 it is the last one).  What the expander did was to put there
>> an uninitialized pseudo, so we ended up with pretty random content, before
>> H.J's http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190492 it happened
>> to work by accident, but when things changed slightly and reload chose
>> different alternative, this broke.
>>
>> 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).
>
> Can we introduce additional "*fmai_fmadd_<mode>_1" pattern (and
> others) that would cover missing 231 alternative?
>
>> 2012-09-13  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR target/54564
>>         * config/i386/sse.md (fmai_vmfmadd_<mode>): Use (match_dup 1)
>>         instead of (match_dup 0) as second argument to vec_merge.
>>         (*fmai_fmadd_<mode>, *fmai_fmsub_<mode>): Likewise.
>>         Remove third alternative.
>>         (*fmai_fnmadd_<mode>, *fmai_fnmsub_<mode>): Likewise.  Negate
>>         operand 2 instead of operand 1, but put it as first argument
>>         of fma.
>>
>>         * config/i386/fmaintrin.h (_mm_fnmadd_sd, _mm_fnmadd_ss,
>>         _mm_fnmsub_sd, _mm_fnmsub_ss): Negate the second argument instead
>>         of the first.
>
> OK, but header change should be also reviewed by H.J.
>

It looks OK to me.

Thanks.


-- 
H.J.