Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix

[Jan Beulich <jbeulich@suse.com>]

On 09.11.2023 13:31, Cui, Lili wrote:
>> Subject: Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix
>>
>> On 03.11.2023 17:50, Cui, Lili wrote:
>>> --- a/gas/testsuite/gas/i386/x86-64-inval-movbe.l
>>> +++ b/gas/testsuite/gas/i386/x86-64-inval-movbe.l
>>> @@ -1,29 +1,30 @@
>>>  .*: Assembler messages:
>>> -.*:4: Error: .*
>>>  .*:5: Error: .*
>>>  .*:6: Error: .*
>>>  .*:7: Error: .*
>>>  .*:8: Error: .*
>>> -.*:11: Error: .*
>>> +.*:9: Error: .*
>>>  .*:12: Error: .*
>>>  .*:13: Error: .*
>>>  .*:14: Error: .*
>>>  .*:15: Error: .*
>>> +.*:16: Error: .*
>>>  GAS LISTING .*
>>>
>>>
>>>  [ 	]*1[ 	]+\# Check illegal movbe in 64bit mode\.
>>>  [ 	]*2[ 	]+\.text
>>> -[ 	]*3[ 	]+foo:
>>> -[ 	]*4[ 	]+movbe	\(%rcx\),%bl
>>> -[ 	]*5[ 	]+movbe	%ecx,%ebx
>>> -[ 	]*6[ 	]+movbe	%bx,%rcx
>>> -[ 	]*7[ 	]+movbe	%rbx,%rcx
>>> -[ 	]*8[ 	]+movbe	%bl,\(%rcx\)
>>> -[ 	]*9[ 	]+
>>> -[ 	]*10[ 	]+\.intel_syntax noprefix
>>> -[ 	]*11[ 	]+movbe bl, byte ptr \[rcx\]
>>> -[ 	]*12[ 	]+movbe ebx, ecx
>>> -[ 	]*13[ 	]+movbe rcx, bx
>>> -[ 	]*14[ 	]+movbe rcx, rbx
>>> -[ 	]*15[ 	]+movbe byte ptr \[rcx\], bl
>>> +[ 	]*3[ 	]+\.arch \.noapx_f
>>> +[ 	]*4[ 	]+foo:
>>> +[ 	]*5[ 	]+movbe	\(%rcx\),%bl
>>> +[ 	]*6[ 	]+movbe	%ecx,%ebx
>>> +[ 	]*7[ 	]+movbe	%bx,%rcx
>>> +[ 	]*8[ 	]+movbe	%rbx,%rcx
>>> +[ 	]*9[ 	]+movbe	%bl,\(%rcx\)
>>> +[ 	]*10[ 	]+
>>> +[ 	]*11[ 	]+\.intel_syntax noprefix
>>> +[ 	]*12[ 	]+movbe bl, byte ptr \[rcx\]
>>> +[ 	]*13[ 	]+movbe ebx, ecx
>>> +[ 	]*14[ 	]+movbe rcx, bx
>>> +[ 	]*15[ 	]+movbe rcx, rbx
>>> +[ 	]*16[ 	]+movbe byte ptr \[rcx\], bl
>>
>> To avoid the need to fiddle with this file, did you consider changing the test's
>> command line options instead? In any event ...
>>
> 
> Do you mean disabling apx_f with the command line "#as..."?

Yes.

>  I tried "#as -march=+noapx_f" , but not worked.

Listing tests have their command line options passed directly from the
.exp file.

>>> --- a/opcodes/i386-dis-evex-len.h
>>> +++ b/opcodes/i386-dis-evex-len.h
>>> @@ -62,6 +62,16 @@ static const struct dis386 evex_len_table[][3] = {
>>>      { REG_TABLE (REG_EVEX_0F38C7_L_2) },
>>>    },
>>>
>>> +  /* EVEX_LEN_0F38F2 */
>>> +  {
>>> +    { "andnS",		{ Gdq, VexGdq, Edq }, 0 },
>>> +  },
>>
>> There's no sign of a prefix decode step here.
>>
> 
> The prefix decoding step is in the NF patch and its dependent patches (Part II 2/6). Both are suspended currently.

But prefix decoding is orthogonal to NF handling. Why would that
step be added only there?

>>> --- a/opcodes/i386-dis-evex-mod.h
>>> +++ b/opcodes/i386-dis-evex-mod.h
>>> @@ -1 +1,43 @@
>>>  /* Nothing at present.  */
>>
>> This comment needs to go away when new stuff is added here. However, I'm
>> sure I requested before that no new entries be put here which have only one
>> of their two slots populated. The reg-only and mem-only aspects can be
>> expressed via proper choice of operand specifiers, at least in almost all cases.
>> Note how you already use ...
>>
>>> +  /* MOD_EVEX_MAP4_DA_PREFIX_1 */
>>> +  {
>>> +    { Bad_Opcode },
>>> +    { "encodekey128", { Gd, Ed }, 0 },  },
>>> +  /* MOD_EVEX_MAP4_DB_PREFIX_1 */
>>> +  {
>>> +    { Bad_Opcode },
>>> +    { "encodekey256", { Gd, Ed }, 0 },  },
>>> +  /* MOD_EVEX_MAP4_DC_PREFIX_1 */
>>> +  {
>>> +    { "aesenc128kl",    { XM, M }, 0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_DD_PREFIX_1 */
>>> +  {
>>> +    { "aesdec128kl",    { XM, M }, 0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_DE_PREFIX_1 */
>>> +  {
>>> +    { "aesenc256kl",    { XM, M }, 0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_DF_PREFIX_1 */
>>> +  {
>>> +    { "aesdec256kl",    { XM, M }, 0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_F8_PREFIX_1 */
>>> +  {
>>> +    { "enqcmds",	{ Gva, M },  0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_F8_PREFIX_2 */
>>> +  {
>>> +    { "movdir64b",	{ Gva, M }, 0 },
>>> +  },
>>> +  /* MOD_EVEX_MAP4_F8_PREFIX_3 */
>>> +  {
>>> +    { "enqcmd",		{ Gva, M }, 0 },
>>> +  },
>>
>> ... M in many entries anyway. These can move one level up without needing
>> further adjustment.
>>
>> For all of the above, however, an EVEX.W decode step looks to be missing.
>> Interestingly the doc consistently omits the (presumably) .W0 suffix for these,
>> having merely a trailing dot there. The issue (doc and/or code) is present
>> elsewhere as well.
>>
> 
> Do you mean that all EVEX promoted instructions need to be explicitly .W0 in
> the doc?

Not exactly, no. The doc should explicitly state what the behavior wrt
EVEX.W is. I'm merely guessing that for the insns where it's missing
W0 might be meant (on the assumption that a common goal ought to be to
waste as little encoding space as possible). If it's WIG (in the present
doc typically spelled IGNORED) instead, ...

> in the case of, I'll handle it uniformly in get_valid_dis386.

... no W decoding step would be needed of course.

Jan