Re: [PATCH 4/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

On 22.10.2023 16:05, Cui, Lili wrote:
>>> @@ -8825,6 +8853,9 @@ build_modrm_byte (void)
>>>        break;
>>>    if (v >= dest)
>>>      v = ~0;
>>> +  if (i.tm.opcode_space == SPACE_EVEXMAP4
>>> +      && i.tm.opcode_modifier.vexvvvv)
>>> +    v = dest;
>>>    if (i.tm.extension_opcode != None)
>>>      {
>>>        if (dest != source)
>>> @@ -9088,6 +9119,9 @@ build_modrm_byte (void)
>>>        set_rex_vrex (i.op[op].regs, REX_B, false);
>>>  	}
>>>
>>> +      if (i.tm.opcode_space == SPACE_EVEXMAP4
>>> +	  && i.tm.opcode_modifier.vexvvvv)
>>> +	dest--;
>>>        if (op == dest)
>>>  	dest = ~0;
>>>        if (op == source)
>>
>> These two changes are at the very least problematic with .insn, whose
>> behavior may not change. I'd also prefer if we could get away with just one
>> change to the function. Did you consider alternatives? We could re- widen
>> VexVVVV, such that the value 2 indicates that the destination is encoded there.
>> That then also has no chance of conflicting with .insn.
>>
> I added value 2 for NDD, if it's ok, I will create another patch to move  (i.tm.extension_opcode != None) to VexVVVVDEST branch, and use value 3 instead of SWAP_SOURCES, maybe name it VexVVVVSRC1, or just VexVVVVOP1, VexVVVVOP2 and VexVVVVOP3?

Hard to tell without actually seeing the code. What you did quote below
used just SRC/DEST.

>   /* How to encode VEX.vvvv:
>      0: VEX.vvvv must be 1111b.
>      1: VEX.vvvv encodes one of the register operands.
>      2: VEX.vvvv encodes as the dest register operands.
>    */
> #define VexVVVVSRC   1
> #define VexVVVVDEST  2
>   VexVVVV,

For readability maybe insert underscores (and if SRC remains un-numbered,
maybe also DST in place of DEST)?

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd.s
>>> @@ -0,0 +1,156 @@
>>> +# Check 64bit APX NDD instructions with evex prefix encoding
>>> +
>>> +	.allow_index_reg
>>> +	.text
>>> +_start:
>>> +cmovge 0x90909090(%eax),%edx,%r8d
>>> +cmovle 0x90909090(%eax),%edx,%r8d
>>> +cmovg  0x90909090(%eax),%edx,%r8d
>>> +imul   0x90909(%eax),%edx,%r8d
>>> +imul   0x909(%rax,%r31,8),%rdx,%r25
>>
>> What about imul by immediate? The present spec is quite unclear there:
>> The insn page says {ND=ZU} and the table says 0/1 in the ND column.
>>
> 
> We don't support it yet, I put it in RFC.
> ...
> 2. Support APX ZU   -- In progress
> 3. Support APX CCMP and CTEST -- In progress
> ...
> 
> About 0/1 in the ND column, it means ZU can be 0/1.
> 
> IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
> Although these instructions do not support NDD, the EVEX.ND bit is used to control whether its
> destination register has its upper bits (namely, bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
> That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise, they keep the old values
> when OSIZE is 8b or 16b. For these instructions, EVEX.[V4,V3,V2,V1,V0] must be all zero.

So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is EVEX.ND
then simply being ignored? The ZU really is meaningful only for 16-bit forms,
aiui ...

>>> +.byte 0x62,0xf4,0xfc,0x08,0xff,0xc0  #inc %rax .byte
>>> +0x62,0xf4,0xec,0x08,0xff,0xc0  #bad
>>
>> As before, please avoid .byte whenever possible. And please have a more
>> detailed comment as to what is being encoded, when .byte cannot be avoided.
>> Plus, if at all possible, have "bad" tests live in separate testcases from "good"
>> ones.
>>
> 
> This case wants to test that inc supports evex format without GPR32,  patch part II 1/6 will cover it. The first case has been removed and the second case has been added to x86-64-apx-evex-promoted-bad.s.

That's all fine, but still wants expressing with .insn rather than .byte,
if at all possible.

Jan