2016-11-11 22:14 GMT+03:00 Uros Bizjak <ubiz...@gmail.com>:
> On Fri, Nov 11, 2016 at 7:23 PM, Andrew Senkevich
> <andrew.n.senkev...@gmail.com> wrote:
>> 2016-11-11 20:56 GMT+03:00 Uros Bizjak <ubiz...@gmail.com>:
>>> 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.
>>
>> Changed according your comments and attached.
>
> Still not good.
>
> +(define_insn "kmovd"
> + [(set (match_operand:SI 0 "register_operand" "=k,k")
> + (unspec:SI
> + [(match_operand:SI 1 "nonimmediate_operand" "r,km")]
> + UNSPEC_KMOV))]
> + "TARGET_AVX512BW && !MEM_P (operands[1])"
>
> Remove !MEM_P in the above pattern.
>
> (define_insn "kmovw"
> - [(set (match_operand:HI 0 "nonimmediate_operand" "=k,k")
> + [(set (match_operand:HI 0 "register_operand" "=k,k")
> (unspec:HI
> [(match_operand:HI 1 "nonimmediate_operand" "r,km")]
> UNSPEC_KMOV))]
> - "!(MEM_P (operands[0]) && MEM_P (operands[1])) && TARGET_AVX512F"
> + "TARGET_AVX512F && !MEM_P (operands[1])"
>
> Also remove !MEM_P here.
>
> +(define_insn "kadd<mode>"
> + [(set (match_operand:SWI1248x 0 "register_operand" "=r,&r,!k")
> + (plus:SWI1248x
> + (not:SWI1248x
> + (match_operand:SWI1248x 1 "register_operand" "r,0,k"))
> + (match_operand:SWI1248x 2 "register_operand" "r,r,k")))
> + (clobber (reg:CC FLAGS_REG))]
> + "TARGET_AVX512F"
> +{
> + switch (which_alternative)
> + {
> + case 0:
> + return "add\t{%k2, %k1, %k0|%k0, %k1, %k2}";
> + case 1:
> + return "#";
> + case 2:
> + if (TARGET_AVX512BW && <MODE>mode == DImode)
> + return "kaddq\t{%2, %1, %0|%0, %1, %2}";
> + else if (TARGET_AVX512BW && <MODE>mode == SImode)
> + return "kaddd\t{%2, %1, %0|%0, %1, %2}";
> + else if (TARGET_AVX512DQ && <MODE>mode == QImode)
> + return "kaddb\t{%2, %1, %0|%0, %1, %2}";
> + else
> + return "kaddw\t{%2, %1, %0|%0, %1, %2}";
> +
>
> The above pattern is wrong. Is there really a NOT RTX present,
> implying effectively a kaddn?
>
> If this is plain add, then you need to change other add patterns, see
> how logic patterns are amended with "k" constraint, added pattern
> should look like *k<logic><mode> pattern.
>
> (define_insn "kandn<mode>"
> - [(set (match_operand:SWI12 0 "register_operand" "=r,&r,!k")
> - (and:SWI12
> - (not:SWI12
> - (match_operand:SWI12 1 "register_operand" "r,0,k"))
> - (match_operand:SWI12 2 "register_operand" "r,r,k")))
> + [(set (match_operand:SWI1248x 0 "register_operand" "=r,&r,!k")
> + (and:SWI1248x
> + (not:SWI1248x
> + (match_operand:SWI1248x 1 "register_operand" "r,0,k"))
> + (match_operand:SWI1248x 2 "register_operand" "r,r,k")))
> (clobber (reg:CC FLAGS_REG))]
> "TARGET_AVX512F"
> {
> @@ -8319,10 +8358,50 @@
> case 1:
> return "#";
> case 2:
> - if (TARGET_AVX512DQ && <MODE>mode == QImode)
> + if (TARGET_AVX512BW && <MODE>mode == DImode)
> + return "kandnq\t{%2, %1, %0|%0, %1, %2}";
> + else if (TARGET_AVX512BW && <MODE>mode == SImode)
> + return "kandnd\t{%2, %1, %0|%0, %1, %2}";
> + else if (TARGET_AVX512DQ && <MODE>mode == QImode)
> return "kandnb\t{%2, %1, %0|%0, %1, %2}";
> else
> return "kandnw\t{%2, %1, %0|%0, %1, %2}";
>
> The above should use SWI1248_AVX512BW mode iterator, see
> *k<logic><mode> pattern.I split this patch after last updates in md files, here is the first part which doesn't change md files. Regtested on x86_64-linux-gnu. Is this part ok? -- WBR, Andrew
avx512-kmask-intrin-part1.patch
Description: Binary data
