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. Thanks, Uros.