Re: [PATCH] Avoid (almost) ix86_binary_operator_ok in sse.md (PR target/82855)

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Nov 7, 2017 at 8:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 06, 2017 at 11:27:27PM +0100, Uros Bizjak wrote:
>> On Mon, Nov 6, 2017 at 10:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > Hi!
>> >
>> > As this patch shows, we have tons of ix86_binary_operator_ok calls
>> > in sse.md patterns, but I believe those are inappropriate in all these
>> > spots, the function is for normal 2 operand binary instructions, where
>> > we require that if one operand is memory, the destination is as well and
>> > they match.  That is generally not the case for 3 operand SSE*/AVX*
>> > instructions, where the destination is always a register, and one of the
>> > other operands can be a memory.  All we care about and is what we express
>> > in condition on many other sse.md instructions is that at most one input
>> > operand is a memory, never both.
>>
>> !(MEM_P (operands[1]) && MEM_P (operands[2])) applies to AVX patterns
>> only. Please note that ix86_binary_operator_ok also handles dst/src1
>> operand matching and commutative operands in non-AVX SSE patterns.
>> Looking at the patch, perhaps we should introduce
>> ix86_fixup_sse_binary_operator and ix86_sse_binary_operator_ok that
>> would bypass most of ix86_{fixup_,}binary_operator{,_ok} operand
>> handling for TARGET_AVX? This way, we will still allow memory operands
>> as operand1 and operand2 for commutative operators in AVX and non-AVX
>> patterns and match src1 and dst of non-AVX patterns.
>
> What I mean is that on randomly chosen SSE pattern:
> (define_insn "*<plusminus_insn><mode>3"
>   [(set (match_operand:VI_AVX2 0 "register_operand" "=x,v")
>         (plusminus:VI_AVX2
>           (match_operand:VI_AVX2 1 "vector_operand" "<comm>0,v")
>           (match_operand:VI_AVX2 2 "vector_operand" "xBm,vm")))]
>   "TARGET_SSE2
>    && ix86_binary_operator_ok (<CODE>, <MODE>mode, operands)"
>   "@
>    p<plusminus_mnemonic><ssemodesuffix>\t{%2, %0|%0, %2}
>    vp<plusminus_mnemonic><ssemodesuffix>\t{%2, %1, %0<mask_operand3>|%0<mask_operand3>, %1, %2}"
> ix86_binary_operator_ok does:
> 1)
>   /* Both source operands cannot be in memory.  */
>   if (MEM_P (src1) && MEM_P (src2))
>     return false;
> This is what we care about
> 2)
>   /* Canonicalize operand order for commutative operators.  */
>   if (ix86_swap_binary_operands_p (code, mode, operands))
>     std::swap (src1, src2);
> Just helper for the further handling if commutative.
> 3)
>   /* If the destination is memory, we must have a matching source operand.  */
>   if (MEM_P (dst) && !rtx_equal_p (dst, src1))
>       return false;
> This is useless, because dst must satisfy register_operand, so will
> never return false.
> 4)
>   /* Source 1 cannot be a constant.  */
>   if (CONSTANT_P (src1))
>     return false;
> This is useless, because both input operands are using
> nonimmediate_operand or vector_operand or similar predicates,
> which don't include immediates.
> 5)
>   /* Source 1 cannot be a non-matching memory.  */
>   if (MEM_P (src1) && !rtx_equal_p (dst, src1))
>     /* Support "andhi/andsi/anddi" as a zero-extending move.  */
>     return (code == AND
>             && (mode == HImode
>                 || mode == SImode
>                 || (TARGET_64BIT && mode == DImode))
>             && satisfies_constraint_L (src2));
> Because dst is not a MEM, this means effectively that if (MEM_P (src1))
> return false; but for commutative patterns 1) together with 2) already
> ensured that (mode is not scalar).

Yes, I am aware that ix86_binary_operator_ok (and corresponding
expander fixup) is way too complex for SSE patterns, but until AVX,
this function handled all patterns in an universal way. Please also
note, that the intention of the corresponding expander fixup function
is to make expanded pattern like final instruction as much as possible
(e.g. emit memory operand of a commutative operator as operand2, or
push memory operands to a register). I believe that recent
improvements in RA made these functions somehow obsolete, but I still
think it is beneficial to fixup memory operands at expansion time,
even for AVX.

> You are right that for non-commutative 5) is needed, but for instructions
> that are never commutative we can and already do ensure it by just
> using register_operand predicate instead of
> nonimmediate_operand/vector_operand (and then even 1) isn't needed
> in the condition, because the predicates guarantee it is never true).

True, if there are explicit reg/reg/mem predicates, we don't need any
additional insn constraints. These should be removed.

> For always commutative insns, where we have explicit % in the constraints,
> I believe !(MEM_P (operands[1]) && MEM_P (operands[2])) is also completely
> sufficient.

Probably true, nowadays RA should be able to resolve this. So, if this
works OK, we can also remove a bunch of expanders that fixup memory
arguments.

> The last category are insns with <comm> in constraints, sometimes
> commutative, sometimes not, where !(MEM_P (operands[1]) && MEM_P (operands[2]))
> is insufficent.  I can introduce ix86_{fixup_,}sse_binary_operator{,_ok}
> for those, sure.

Agreed.

> The question is, for the always commutative insns like:
> (define_insn "*mul<mode>3<mask_name><round_name>"
>   [(set (match_operand:VF 0 "register_operand" "=x,v")
>         (mult:VF
>           (match_operand:VF 1 "<round_nimm_predicate>" "%0,v")
>           (match_operand:VF 2 "<round_nimm_predicate>" "xBm,<round_constraint>")))]
>   "TARGET_SSE && ix86_binary_operator_ok (MULT, <MODE>mode, operands) && <mask_mode512bit_condition> && <round_mode512bit_condition>"
> do you prefer !(MEM_P (operands[1]) && MEM_P (operands[2]))
> in the condition, or ix86_fixup_sse_binary_operator_ok even
> when it will in practice do the same thing?

No. As mentioned above, !(MEM_P .... ) should be sufficient.

As you noticed, there are special cases with sse_movhlps, sse_movlhps,
sse_loadhps, sse_loadlps, sse2_loadhpd and sse2_loadlpd. Here we have
to match dst/op1. While we use ix86_fixup_binary_operands in the
corresponding expanders, I think we have to use full-blown
x86_binary_operator_ok in their insn patterns to avoid RA failures.

Uros.