On Fri, Nov 11, 2016 at 6:50 PM, Uros Bizjak <ubiz...@gmail.com> wrote:
> On Fri, Nov 11, 2016 at 6:38 PM, Andrew Senkevich
> <andrew.n.senkev...@gmail.com> wrote:
>> 2016-11-11 17:34 GMT+03:00 Uros Bizjak <ubiz...@gmail.com>:
>>> Some quick remarks:
>>>
>>> +(define_insn "kmovb"
>>> + [(set (match_operand:QI 0 "nonimmediate_operand" "=k,k")
>>> + (unspec:QI
>>> + [(match_operand:QI 1 "nonimmediate_operand" "r,km")]
>>> + UNSPEC_KMOV))]
>>> + "!(MEM_P (operands[0]) && MEM_P (operands[1])) && TARGET_AVX512DQ"
>>> + "@
>>> + kmovb\t{%k1, %0|%0, %k1}
>>> + kmovb\t{%1, %0|%0, %1}";
>>> + [(set_attr "mode" "QI")
>>> + (set_attr "type" "mskmov")
>>> + (set_attr "prefix" "vex")])
>>> +
>>> +(define_insn "kmovd"
>>> + [(set (match_operand:SI 0 "nonimmediate_operand" "=k,k")
>>> + (unspec:SI
>>> + [(match_operand:SI 1 "nonimmediate_operand" "r,km")]
>>> + UNSPEC_KMOV))]
>>> + "!(MEM_P (operands[0]) && MEM_P (operands[1])) && TARGET_AVX512BW"
>>> + "@
>>> + kmovd\t{%k1, %0|%0, %k1}
>>> + kmovd\t{%1, %0|%0, %1}";
>>> + [(set_attr "mode" "SI")
>>> + (set_attr "type" "mskmov")
>>> + (set_attr "prefix" "vex")])
>>> +
>>> +(define_insn "kmovq"
>>> + [(set (match_operand:DI 0 "nonimmediate_operand" "=k,k,km")
>>> + (unspec:DI
>>> + [(match_operand:DI 1 "nonimmediate_operand" "r,km,k")]
>>> + UNSPEC_KMOV))]
>>> + "!(MEM_P (operands[0]) && MEM_P (operands[1])) && TARGET_AVX512BW"
>>> + "@
>>> + kmovq\t{%k1, %0|%0, %k1}
>>> + kmovq\t{%1, %0|%0, %1}
>>> + kmovq\t{%1, %0|%0, %1}";
>>> + [(set_attr "mode" "DI")
>>> + (set_attr "type" "mskmov")
>>> + (set_attr "prefix" "vex")])
>>>
>>> - kmovd (and existing kmovw) should be using register_operand for
>>> opreand 0. In this case, there is no need for MEM_P checks at all.
>>> - In the insn constraint, pease check TARGET_AVX before checking MEM_P.
>>> - please put these definitions above corresponding *mov??_internal patterns.
>>
>> Do you mean put below *mov??_internal patterns? Attached corrected such way.
>
> No, please put kmovq near *movdi_internal, kmovd near *movsi_internal,
> etc. It doesn't matter if they are above or below their respective
> *mov??_internal patterns, as long as they are positioned in some
> consistent way. IOW, new patterns shouldn't be grouped together, as is
> the case with your patch.
+(define_insn "kmovb"
+ [(set (match_operand:QI 0 "register_operand" "=k,k")
+ (unspec:QI
+ [(match_operand:QI 1 "nonimmediate_operand" "r,km")]
+ UNSPEC_KMOV))]
+ "TARGET_AVX512DQ && !MEM_P (operands[1])"
There is no need for !MEM_P, this will prevent memory operand, which
is allowed by constraint "m".
+(define_insn "kmovq"
+ [(set (match_operand:DI 0 "register_operand" "=k,k,km")
+ (unspec:DI
+ [(match_operand:DI 1 "nonimmediate_operand" "r,km,k")]
+ UNSPEC_KMOV))]
+ "TARGET_AVX512BW && !MEM_P (operands[1])"
Operand 0 should have "nonimmediate_operand" predicate. And here you
need && !(MEM_P (op0) && MEM_P (op1)) in insn constraint to prevent
mem->mem moves.
Uros.
