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

[Jakub Jelinek]

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).

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).

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.

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.

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?

	Jakub