Re: [PATCH 5/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

On 20.11.2023 13:36, Cui, Lili wrote:
>> On 02.11.2023 12:29, Cui, Lili wrote:
>>> --- a/opcodes/i386-dis-evex.h
>>> +++ b/opcodes/i386-dis-evex.h
>>> [...]
>>> @@ -947,23 +947,23 @@ static const struct dis386 evex_table[][256] = {
>>>      { Bad_Opcode },
>>>      { Bad_Opcode },
>>>      /* 40 */
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> +    { "cmovoS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovnoS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovbS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovaeS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmoveS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovneS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovbeS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovaS",		{ VexGv, Gv, Ev }, 0 },
>>>      /* 48 */
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> -    { Bad_Opcode },
>>> +    { "cmovsS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovnsS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovpS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovnpS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovlS",		{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovgeS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovleS",	{ VexGv, Gv, Ev }, 0 },
>>> +    { "cmovgS",		{ VexGv, Gv, Ev }, 0 },
>>
>> Considering CFCMOVcc which sits at the same opcode, doing things like this
>> sets us up for needing to touch all of these again. Maybe that's the best that
>> can be done, but I still wonder whether this couldn't be taken care of right
>> away when introducing these entries.
> 
> How about adding a special letter CF% in front of them?

Why not.

>>> --- a/opcodes/i386-dis.c
>>> +++ b/opcodes/i386-dis.c
>>> [...]
>>> @@ -2660,47 +2668,47 @@ static const struct dis386 reg_table[][8] = {
>>>    },
>>>    /* REG_D0 */
>>>    {
>>> -    { "rolA",	{ Eb, I1 }, 0 },
>>> -    { "rorA",	{ Eb, I1 }, 0 },
>>> -    { "rclA",	{ Eb, I1 }, 0 },
>>> -    { "rcrA",	{ Eb, I1 }, 0 },
>>> -    { "shlA",	{ Eb, I1 }, 0 },
>>> -    { "shrA",	{ Eb, I1 }, 0 },
>>> -    { "shlA",	{ Eb, I1 }, 0 },
>>> -    { "sarA",	{ Eb, I1 }, 0 },
>>> +    { "rolA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "rorA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "rclA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "rcrA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "shrA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
>>> +    { "sarA",	{ VexGb, Eb, I1 }, 0 },
>>>    },
>>>    /* REG_D1 */
>>>    {
>>> -    { "rolQ",	{ Ev, I1 }, 0 },
>>> -    { "rorQ",	{ Ev, I1 }, 0 },
>>> -    { "rclQ",	{ Ev, I1 }, 0 },
>>> -    { "rcrQ",	{ Ev, I1 }, 0 },
>>> -    { "shlQ",	{ Ev, I1 }, 0 },
>>> -    { "shrQ",	{ Ev, I1 }, 0 },
>>> -    { "shlQ",	{ Ev, I1 }, 0 },
>>> -    { "sarQ",	{ Ev, I1 }, 0 },
>>> +    { "rolQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "rorQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "rclQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "rcrQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "shrQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
>>> +    { "sarQ",	{ VexGv, Ev, I1 }, 0 },  
>>>    },
>>
>> As mentioned on the assembler side already, I think we would be better off
>> making const_1_mode print $1 in AT&T syntax at least for these new insn
>> forms, to eliminate the ambiguity.
>>
> 
> It is related to correctness and should be revised. Since they share the same entries, I will created a new patch to modify the legacy instruction and then extend them to NDD. Do you agree?

I certainly appreciate any reusing, where it is possible (and it ought to be
possible here, yes).

>>> @@ -9087,7 +9104,7 @@ get_valid_dis386 (const struct dis386 *dp,
>> instr_info *ins)
>>>  	return &err_opcode;
>>>
>>>        /* Set vector length.  */
>>> -      if (ins->modrm.mod == 3 && ins->vex.b)
>>> +      if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type ==
>>> + evex_default)
>>>  	ins->vex.length = 512;
>>>        else
>>>  	{
>>
>> Is this change really needed for anything?
> 
> If it's NDD and ins->vex.b ==1, we need to avoid giving NDD a wrong value. 

But this is recording ->vex.length, not anything NDD related (afaics).

Jan