RE: [PATCH v4 4/9] Add tests for APX GPR32 with extend evex prefix

["Cui, Lili" <lili.cui@intel.com>]

> >>> +	vroundpd $1,(%r24),%xmm6
> >>> +	vroundps $2,(%r24),%xmm6
> >>> +	vroundsd $3,(%r24),%xmm6,%xmm3
> >>> +	vroundss $4,(%r24),%xmm6,%xmm3
> >>
> >> These are still here, when they can be expressed.
> >
> > I think we should keep it until we complete a reasonably equivalent
> replacement.
> 
> This replacement should have been introduced in this series, as asked for more
> than once. And, H.J., I think you shouldn't have approved this patch (and
> more generally any one) with unaddressed review comments.
>

We have different opinion for this part. Currently under discussion.

> >>> --- /dev/null
> >>> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> >>> @@ -0,0 +1,34 @@
> >>> +# Check Illegal prefix for 64bit EVEX-promoted instructions
> >>> +
> >>> +        .allow_index_reg
> >>> +        .text
> >>> +_start:
> >>> +	#movbe %r23w,%ax set EVEX.pp = f3 (illegal value).
> >>> +	.insn EVEX.L0.f3.M12.W0 0x60, %di, %ax
> >>> +	#movbe %r23w,%ax set EVEX.pp = f2 (illegal value).
> >>> +	.insn EVEX.L0.f2.M12.W0 0x60, %di, %ax
> >>> +	#VSIB vpgatherqq (%rbp,%zmm17,8),%zmm16{%k1} set EVEX.P[10]
> >> == 0
> >>> +	#(illegal value).
> >>> +	.byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd
> >>> +	.byte 0xff
> >>
> >> This one's still using .byte and still referencing P[10] in the comment.
> >> If that's really unavoidable, then - as elsewhere - the description
> >> of the patch could (and should) provide the reason. And there
> >> continue to be no separating blank lines, making it as hard as before
> >> to find ones way through all of this.
> >>
> >
> > Copied from the previous comments,
> > ----------------------------------------------------------------------
> > --
> >>>> +	#(illegal value).
> >>>> +	.byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd, 0x7b, 0x00, 0x00,
> 0x00
> >>>> +	.byte 0xff
> >>>
> >>> For the purpose of this test (whatever P[10] again is) you don't
> >>> need a 32-bit displacement, do you? Shorter is (almost always) better in
> such tests.
> >>>
> >>
> >> P[10] is a fixed value, in normal EVEX format we don't use this bit.  Dropped
> 0x7b.
> > ----------------------------------------------------------------------
> > --
> 
> This is not addressing my comment. Iirc all bit in the EVEX prefix have names
> now in SDM plus APX spec, and hence they should preferably be referred to by
> their names rather than their bit positions. Plus there's still no justification for
> using .byte instead of .insn.
> 

I will add EVEX.X4 to it, but for using .insn instead of .byte, I think the current .insn does not support the new EVEX format, I don't know how to express the fixed value P[10] with .insn, maybe you have some good suggestions.f
 (insn is really hard to use, I admit it has advantages for some test expressions, but I don't think it works for all).

> >>> +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax{%k1}
> >>> +	#EVEX_MAP4 movbe %r18w,%ax set EVEX.L'L == 0b01 (illegal value).
> >>> +	.insn EVEX.L1.66.M12.W0 0x60, %di, %ax
> >>> +	#EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1 (illegal value).
> >>> +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k7}{z}
> >>> +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[17:16](EVEX.aa)
> >> == 0b01
> >>> +	#(illegal value).
> >>> +	.insn EVEX.L0.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx{%k1}
> >>> +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%ecx EVEX.P[22:21](EVEX.L’L)
> >> == 0b01
> >>> +	#(illegal value).
> >>> +	.insn EVEX.L1.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx
> >>> +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) ==
> >> 0b1
> >>> +	#(illegal value).
> >>> +        .insn EVEX.L0.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx
> >>> +{%k7}{z}
> >>
> >> Isn't {%k7} alone rendering the encoding invalid, which was checked
> >> for above already? Also - bogus indentation again.
> >>
> >
> > That is for legacy, this is for vex.
> 
> What is "that" here? And how can anything with operand mask applied be
> VEX? The more that .insn's operand says EVEX? Here you want to set _only_
> EVEX.z, without EVEX.aaa being non-zero.
> 

These two test cases,  one is for evex form legacy, another is for evex from vex.

        #EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1.
        .insn EVEX.L0.66.M12.W0 0x60, %di, %ax{k7}{z}

        #EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) == 0b1
        .insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k7}{z}

If I just put {z} here, I will get the error: "Only write masking is allowed for clear masking.".  So, I had to add {k7}

Lili.