On Mon, Nov 06, 2017 at 11:27:27PM +0100, Uros Bizjak wrote: > On Mon, Nov 6, 2017 at 10:18 PM, Jakub Jelinek <ja...@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