[PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 02.07.2024 15:49, Cui, Lili wrote:
>> On 02.07.2024 10:22, Jan Beulich wrote:
>>> On 02.07.2024 09:09, Cui, Lili wrote:
>>>>> On 01.07.2024 15:42, Cui, Lili wrote:
>>>>>> I met some issues when adding CTESTcc and CCMPcc.
>>>>>>
>>>>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now
>>>>>> looks a bit
>>>>> weird.
>>>>>>
>>>>>> "ccmpbl {dfv=}.s %edx,%eax"
>>>>>
>>>>> Well ... Something clearly needs doing about this. I'm now even more
>>>>> convinced we want tests for these.
>>>>>
>>>> Indeed, I added a macro %SW to indicate operands were swapped when
>> suffix_always is true.  Are you ok with it?
>>>
>>> In principle (and if then used consistently), why not. However, as
>>> long as you use it only for ...
>>>
>>>> --- a/opcodes/i386-dis-evex.h
>>>> +++ b/opcodes/i386-dis-evex.h
>>>> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
>>>>      /* 38 */
>>>>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
>>>>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
>>>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
>>>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
>>>> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
>>>> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
>>>
>>> ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?
>>
>> I'm sorry, this was half rubbish, I'm afraid. TEST (and hence CTESTcc) has only
>> a single encoding, and hence would never get .s suffixes. So if there's no better
>> way, a new %SW macro (just for CCMPcc as you have it above) it is then. An
>> alternative might be to convert %DF to a DFV_Fixup() handler (to defer that
>> output enough so that normal .s printing would come first), yet that may be
>> more overhead than what we have right now for the {dfv=...} printing.
>>
> 
> I'm trying to get the opcode from ins->codep and if the opcode > 39 and Msuffix is ​​true, add a ".s" to it.  Do you like this version ?

I'm afraid I don't, since I consider ...

> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> +    { "%NEccmp%SCB%DF",                { Gb, Eb }, 0 },
> +    { "%NEccmp%SCS%DF",                { Gv, Ev }, PREFIX_NP_OR_DATA },
>      { Bad_Opcode },
>      { Bad_Opcode },
>      { Bad_Opcode },
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -10587,6 +10589,11 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>             }
>           else if (l == 1 && last[0] == 'D')
>             {
> +             if (sizeflag & SUFFIX_ALWAYS && (ins->codep[-1] == 0X3A ||ins->codep[-1] == 0X3B))

... this extremely fragile. It was on more than one occasion that I
thought it might be helpful to know the major opcode while handling
operands, but each time I found another solution. First and foremost
to avoid doing what you do here. If we really want to allow access
to the major opcode, it'll need properly storing (along with encoding
space) in "ins".

As a minor remark (may save a comment on a later version), I also
have stylistic concerns with that line (beyond it apparently being
too long). Please can such be written as e.g.

             if ((sizeflag & SUFFIX_ALWAYS) && (ins->codep[-1] | 1) == 0x3B))

?

Jan