FW: [PATCH 3/8] Add tests for APX GPR32 with extend evex prefix
Cui, Lili
lili.cui@intel.com
Tue Oct 17 15:53:28 GMT 2023
> > + packusdw (%r21),%xmm6
> > + pblendvb %xmm0,(%r22),%xmm6
> > + pblendvb (%r22),%xmm6
> > + pblendw $100,(%r22),%xmm6
> > + pcmpeqq (%r22),%xmm6
> > + pextrb $100,%xmm4,(%r22)
> > + pextrw $100,%xmm4,(%r22)
> > + pextrb $100,%xmm4,%r22
>
> Would be nice to stick to strict alphabetic sorting.
>
Done.
> > + pextrd $100,%xmm4,%r22d
> > + pextrd $100,%xmm4,(%r22)
>
> pextrq?
Added.
>
> > + phminposuw (%r23),%xmm4
> > + pinsrb $100,%r23,%xmm4
> > + pinsrb $100,(%r23),%xmm4
> > + pinsrd $100, %r23d, %xmm4
> > + pinsrd $100,(%r23),%xmm4
> > + pinsrq $100, %r24, %xmm4
> > + pinsrq $100,(%r24),%xmm4
> > + pmaxsb (%r24),%xmm6
> > + pmaxsd (%r24),%xmm6
> > + pmaxud (%r24),%xmm6
> > + pmaxuw (%r24),%xmm6
> > + pminsb (%r24),%xmm6
> > + pminsd (%r24),%xmm6
> > + pminud (%r24),%xmm6
> > + pminuw (%r24),%xmm6
> > + pmovsxbw (%r24),%xmm4
> > + pmovsxbd (%r24),%xmm4
> > + pmovsxbq (%r24),%xmm4
> > + pmovsxwd (%r24),%xmm4
> > + pmovsxwq (%r24),%xmm4
> > + pmovsxdq (%r24),%xmm4
> > + pmovsxbw (%r24),%xmm4
> > + pmovzxbd (%r24),%xmm4
> > + pmovzxbq (%r24),%xmm4
> > + pmovzxwd (%r24),%xmm4
> > + pmovzxwq (%r24),%xmm4
> > + pmovzxdq (%r24),%xmm4
> > + pmuldq (%r24),%xmm4
> > + pmulld (%r24),%xmm4
> > + roundpd $100,(%r24),%xmm6
> > + roundps $100,(%r24),%xmm6
> > + roundsd $100,(%r24),%xmm6
> > + roundss $100,(%r24),%xmm6
> > + pcmpgtq (%r25),%xmm4
> > + pcmpestri $100,(%r25),%xmm6
> > + pcmpestrm $100,(%r25),%xmm6
> > + pcmpistri $100,(%r25),%xmm6
> > + pcmpistrm $100,(%r25),%xmm6
>
> For these last five - again would be nice to stick to strict alphabetic sorting.
> (I was almost going to as where pcmpeqq is.)
>
Done.
> > +#AES
> > + aesdec (%r26),%xmm6
> > + aesdeclast (%r26),%xmm6
> > + aesenc (%r26),%xmm6
> > + aesenclast (%r26),%xmm6
> > + aesimc (%r26),%xmm6
> > + aeskeygenassist $100,(%r26),%xmm6
> > + pclmulqdq $100,(%r26),%xmm6
> > + pclmullqlqdq (%r26),%xmm6
> > + pclmulhqlqdq (%r26),%xmm6
> > + pclmullqhqdq (%r26),%xmm6
> > + pclmulhqhqdq (%r26),%xmm6
> > +#GFNI
> > + gf2p8affineqb $100,(%r26),%xmm6
> > + gf2p8affineinvqb $100,(%r26),%xmm6
> > + gf2p8mulb (%r26),%xmm6
> > +#VEX without evex
> > + vblendpd $7,(%r27),%xmm6,%xmm2
> > + vblendpd $7,(%r27),%ymm6,%ymm2
> > + vblendps $7,(%r27),%xmm6,%xmm2
> > + vblendps $7,(%r27),%ymm6,%ymm2
> > + vblendvpd %xmm4,(%r27),%xmm2,%xmm7
> > + vblendvpd %ymm4,(%r27),%ymm2,%ymm7
> > + vblendvps %xmm4,(%r27),%xmm2,%xmm7
> > + vblendvps %ymm4,(%r27),%ymm2,%ymm7
> > + vdppd $7,(%r27),%xmm6,%xmm2
> > + vdpps $7,(%r27),%xmm6,%xmm2
> > + vdpps $7,(%r27),%ymm6,%ymm2
> > + vhaddpd (%r27),%xmm6,%xmm5
> > + vhaddpd (%r27),%ymm6,%ymm5
> > + vhsubps (%r27),%xmm6,%xmm5
> > + vhsubps (%r27),%ymm6,%ymm5
> > + vlddqu (%r27),%xmm4
> > + vlddqu (%r27),%ymm4
> > + vldmxcsr (%r27)
>
> Are the designers not intending to reconsider this (and vstmxcsr), getting on
> par with {ld,st}tilecfg? Even more generally I wonder if things shouldn't remain
> less inconsistent. After all most major opcodes haven't been re-used, so
> adding (less-than-512-bit) EVEX equivalents would only seem reasonable to
> me. I'll be really curious how this asymmetry is going to be dealt with in gcc, in
> a way still permitting sane inline assembly use.
>
For the memory conditions listed here, we may not consider extending them, but for vstmxcsr and vldmxcsr,
I raised a topic for internal discussion. The conclusion is that when apx_f is enabled, gcc will select LDMXCSR/LDMXCSR.
In GCC, we control whether each instruction supports gpr32. GCC default configuration disables gpr32 support in the inline assembler, but users can enable it and need to make sure it is correct.
> > + vmaskmovpd (%r27),%xmm4,%xmm6
> > + vmaskmovpd %xmm4,%xmm6,(%r27)
> > + vmaskmovps (%r27),%xmm4,%xmm6
> > + vmaskmovps %xmm4,%xmm6,(%r27)
> > + vmaskmovpd (%r27),%ymm4,%ymm6
> > + vmaskmovpd %ymm4,%ymm6,(%r27)
> > + vmaskmovps (%r27),%ymm4,%ymm6
> > + vmaskmovps %ymm4,%ymm6,(%r27)
> > + vmovmskpd %xmm4,%r27d
> > + vmovmskpd %xmm8,%r27d
> > + vmovmskps %xmm4,%r27d
> > + vmovmskps %ymm8,%r27d
> > + vpblendvb %xmm4,(%r27),%xmm2,%xmm7
> > + vpblendvb %ymm4,(%r27),%ymm2,%ymm7
> > + vpblendw $7,(%r27),%xmm6,%xmm2
> > + vpblendw $7,(%r27),%ymm6,%ymm2
> > + vpcmpestri $7,(%r27),%xmm6
> > + vpcmpestrm $7,(%r27),%xmm6
> > + vperm2f128 $7,(%r27),%ymm6,%ymm2
> > + vphaddd (%r27),%xmm6,%xmm7
> > + vphaddsw (%r27),%xmm6,%xmm7
> > + vphaddw (%r27),%xmm6,%xmm7
> > + vphsubd (%r27),%xmm6,%xmm7
> > + vphsubsw (%r27),%xmm6,%xmm7
> > + vphsubw (%r27),%xmm6,%xmm7
> > + vphaddd (%r27),%ymm6,%ymm7
> > + vphaddsw (%r27),%ymm6,%ymm7
> > + vphaddw (%r27),%ymm6,%ymm7
> > + vphsubd (%r27),%ymm6,%ymm7
> > + vphsubsw (%r27),%ymm6,%ymm7
> > + vphsubw (%r27),%ymm6,%ymm7
> > + vphminposuw (%r27),%xmm6
> > + vpmovmskb %xmm4,%r27
> > + vpmovmskb %ymm4,%r27d
> > + vpsignb (%r27),%xmm6,%xmm7
> > + vpsignw (%r27),%xmm6,%xmm7
> > + vpsignd (%r27),%xmm6,%xmm7
> > + vpsignb (%r27),%xmm6,%xmm7
> > + vpsignw (%r27),%xmm6,%xmm7
> > + vpsignd (%r27),%xmm6,%xmm7
> > + vptest (%r27),%xmm6
> > + vptest (%r27),%ymm6
> > + vrcpps (%r27),%xmm6
> > + vrcpps (%r27),%ymm6
> > + vrcpss (%r27),%xmm6,%xmm6
> > + vrsqrtps (%r27),%xmm6
> > + vrsqrtps (%r27),%ymm6
> > + vrsqrtss (%r27),%xmm6,%xmm6
> > + vstmxcsr (%r27)
> > + vtestps (%r27),%xmm6
> > + vtestps (%r27),%ymm6
> > + vtestpd (%r27),%xmm6
> > + vtestps (%r27),%ymm6
> > + vtestpd (%r27),%ymm6
> > + vpblendd $7,(%r27),%xmm6,%xmm2
> > + vpblendd $7,(%r27),%ymm6,%ymm2
> > + vperm2i128 $7,(%r27),%ymm6,%ymm2
> > + vpmaskmovd (%r27),%xmm4,%xmm6
> > + vpmaskmovd %xmm4,%xmm6,(%r27)
> > + vpmaskmovq (%r27),%xmm4,%xmm6
> > + vpmaskmovq %xmm4,%xmm6,(%r27)
> > + vpmaskmovd (%r27),%ymm4,%ymm6
> > + vpmaskmovd %ymm4,%ymm6,(%r27)
> > + vpmaskmovq (%r27),%ymm4,%ymm6
> > + vpmaskmovq %ymm4,%ymm6,(%r27)
> > + vaesimc (%r27), %xmm3
> > + vaeskeygenassist $7,(%r27),%xmm3
> > + vroundpd $100,(%r24),%xmm6
> > + vroundps $100,(%r24),%xmm6
> > + vroundsd $100,(%r24),%xmm6,%xmm3
> > + vroundss $100,(%r24),%xmm6,%xmm3
>
> For these I wonder if they shouldn't be accepted (with in-range immediate)
> and converted to vrndscale*. Imo the spec could even mandate (or at least
> suggest) this.
>
Changed it to 0x00 ~ 0x0f.
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-promote-inval.s
> > @@ -0,0 +1,18 @@
> > +# Check illegal 64bit APX EVEX promoted instructions
> > + .text
> > + .arch .apx_f
>
> This doesn't have any effect (on this test), does it?
>
Deleted.
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-egpr.s
> > @@ -0,0 +1,25 @@
> > +# Check 64bit old evex instructions use gpr32 with evex prefix
> > +encoding
> > +
> > + .allow_index_reg
> > + .text
> > +_start:
> > +## MRMDestMem
> > + vextractf32x4 $1, %zmm0, (%r16,%r17)
> > +## MRMSrcMem
> > + vbroadcasti32x4 (%r16,%r17), %zmm0
> > +## MRM0m
> > + vprorq $0, (%r16,%r17), %zmm0
> > +## MRM1m
> > + vprolq $0, (%r16,%r17), %zmm0
> > +## MRM2m
> > + vpsrlq $0, (%r16,%r17), %zmm0
> > +## MRM3m
> > + vpsrldq $0, (%r16,%r17), %zmm0
> > +## MRM4m
> > + vpsraq $0, (%r16,%r17), %zmm0
> > +## MRM6m
> > + vpsllq $0, (%r16,%r17), %zmm0
> > +## MRM7m
> > + vpslldq $0, (%r16,%r17), %zmm0
> > +## MRMDestReg
> > + vextractps $1, %xmm16, %r16d
>
> What is the purpose of this test? Using all the same base and index registers
> isn't really covering very much. I'm also missing a GPR-is- source case (e.g.
> vpinsr*). And finally (I think I commented on this elsewhere already) can the
> comments please be made meaningful, without needing to guess what the
> acronyms are to be decoded to?
>
Removed these comments and added new testcases.
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted.s
> > @@ -0,0 +1,1464 @@
> > +# Check 64bit APX_F EVEX-Promoted instructions.
> > +
> > + .text
> > +_start:
> > + aadd %r25d,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + aadd %r31,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + aand %r25d,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + aand %r31,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc $0x7b,%r16b #APX_F OPC_EVEX_EVEX
> > + adc $0x7b,%r18w #APX_F OPC_EVEX_EVEX
> > + adc $0x7b,%r25d #APX_F OPC_EVEX_EVEX
> > + adc $0x7b,%r31 #APX_F OPC_EVEX_EVEX
> > + adcb $0x7b,0x123(%r16,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcw $0x7b,0x123(%r16,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcl $0x7b,0x123(%r16,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcq $0x7b,0x123(%r16,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcw $0x7b,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcl $0x7b,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adcq $0x7b,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
>
> If this 2nd triplet of adc* is useful, why no adcb? Also, like above:
> - more variation of register use would imo be better (there's no egpr
> index register here),
> - comments want to be useful, or be dropped.
>
Deleted these tests ( Please refer to the next comment for details), and deleted comments.
> Speaking of comments, ...
>
> > + adc %r16b,%dl #APX_F OPC_EVEX_EVEX
> > + adc %r16b,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc %r18w,%ax #APX_F OPC_EVEX_EVEX
> > + adc %r18w,0x123(%r16,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc %r18w,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc %r25d,%edx #APX_F OPC_EVEX_EVEX
> > + adc %r25d,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc %r31,%r15 #APX_F OPC_EVEX_EVEX
> > + adc %r31,0x123(%r31,%rax,4) #APX_F OPC_EVEX_EVEX
> > + adc 0x123(%r16,%rax,4),%r16b #APX_F OPC_EVEX_EVEX
> > + adc 0x123(%r16,%rax,4),%r31 #APX_F OPC_EVEX_EVEX
> > + adc 0x123(%r31,%rax,4),%r18w #APX_F OPC_EVEX_EVEX
> > + adc 0x123(%r31,%rax,4),%r25d #APX_F OPC_EVEX_EVEX
> > + adc 0x123(%r31,%rax,4),%r31 #APX_F OPC_EVEX_EVEX
>
> ... the entire set of adc here doesn't encode as EVEX, but as REX2 (i.e.
> the comments are outright wrong). As a result the EVEX encodings added to
> the opcode table in the previous patch aren't being tested at all.
>
Since their decoding is in NDD, and print {evex} is supported in Part II patch 1/6, these instructions were re-tested in part II patch1/6. Maybe we need to add some egpr32 tests for them in that patch. Deleted all REX2 testcases in this patch.
> > --- a/gas/testsuite/gas/i386/x86-64-evex.d
> > +++ b/gas/testsuite/gas/i386/x86-64-evex.d
> > @@ -17,6 +17,6 @@ Disassembly of section .text:
> > +[a-f0-9]+: 62 f1 d6 38 7b f0 vcvtusi2ss %rax,\{rd-
> sae\},%xmm5,%xmm6
> > +[a-f0-9]+: 62 f1 57 38 7b f0 vcvtusi2sd %eax,\{rd-
> bad\},%xmm5,%xmm6
> > +[a-f0-9]+: 62 f1 d7 38 7b f0 vcvtusi2sd %rax,\{rd-
> sae\},%xmm5,%xmm6
> > - +[a-f0-9]+: 62 e1 7e 08 2d c0 vcvtss2si %xmm0,\(bad\)
> > + +[a-f0-9]+: 62 e1 7e 08 2d c0 vcvtss2si %xmm0,%r16d
> > +[a-f0-9]+: 62 e1 7c 08 c2 c0 00 vcmpeqps %xmm0,%xmm0,\(bad\)
>
> If the line that's now disassembling okay cannot be replaced by a suitable
> equivalent, it should be dropped rather than be adjusted. Furthermore such
> an adjustment needs to be done right in the patch leading to the changed
> output, or else things end up non-bisectable. (New tests are okay to add in a
> separate patch.)
>
Ok, added it to patch 2/8.
> > --- a/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> > +++ b/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> > @@ -1,5 +1,6 @@
> > # Check illegal movbe in 64bit mode.
> > .text
> > + .arch .noapx_f
> > foo:
> > movbe (%rcx),%bl
> > movbe %ecx,%ebx
>
> I don't understand the need for this addition (and hence for the need to
> change the test's expecations). Like was mentioned on the original
> AVX10 series, tests like this shall not need modification, or else it indicates
> people's code also may need ".arch .noapx_f" additions, which I'm sure you
> agree may not be required. Finally, if testcase expecations like the above
> would be needed anywhere, please generalize them such that a similar mere
> addition of a line doesn't require the entire test to be touched. Here this
> means that while for the diagnostics you of course want exact line number
> matches, for the actual listing line numbers don't don't need matching
> individually.
>
Agree with you, but movbe is special, movbe didn't support reg to reg before, but APX enable it. so I added .arch .noapx_f for this invalid test.
Thanks,
Lili
More information about the Binutils
mailing list