2016-12-02 21:31 GMT+03:00 Uros Bizjak <ubiz...@gmail.com>:
> On Fri, Dec 2, 2016 at 6:44 PM, Andrew Senkevich
> <andrew.n.senkev...@gmail.com> wrote:
>> 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?
>
> There is no point to scan for kmovX insn in e.g.:
>
> +/* { dg-final { scan-assembler-times "kmovq" 2 } } */
> +
> +#include <immintrin.h>
> +
> +void
> +avx512bw_test ()
> +{
> + __mmask64 k1, k2, k3;
> + volatile __m512i x = _mm512_setzero_si512 ();
> +
> + __asm__( "kmovq %1, %0" : "=k" (k1) : "r" (1) );
> + __asm__( "kmovq %1, %0" : "=k" (k2) : "r" (2) );
>
> since you emit it from inline asm.
>
> Please remove these pointles kmovX scan-asm-times directives from the
> testcases, and please also remove it from avx512f-kandnw-1.c
> testcase.
>
> The patch is OK with this change.Attached fixed with updated ChangeLogs. HJ, could you commit please? -- WBR, Andrew
avx512-kmask-intrin-part1_v2.patch
Description: Binary data
