Re: FW: [PATCH 1/2] [PATCH 1/2] Enable Intel AVX512_FP16 instructions

[Jan Beulich <jbeulich@suse.com>]

On 20.07.2021 15:38, Cui, Lili wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, July 20, 2021 9:03 PM
>> To: Cui, Lili <lili.cui@intel.com>
>> Cc: hjl.tools@gmail.com; binutils@sourceware.org
>> Subject: Re: FW: [PATCH 1/2] [PATCH 1/2] Enable Intel AVX512_FP16
>> instructions
>>
>> On 20.07.2021 13:26, Cui, Lili wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Tuesday, July 20, 2021 4:46 PM
>>>> To: Cui, Lili <lili.cui@intel.com>
>>>> Cc: hjl.tools@gmail.com; binutils@sourceware.org
>>>> Subject: Re: FW: [PATCH 1/2] [PATCH 1/2] Enable Intel AVX512_FP16
>>>> instructions
>>>>
>>>> On 20.07.2021 09:08, Cui, Lili wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: Wednesday, July 14, 2021 11:21 PM
>>>>>> To: Cui, Lili <lili.cui@intel.com>
>>>>>> Cc: hjl.tools@gmail.com; binutils@sourceware.org
>>>>>> Subject: Re: [PATCH 1/2] [PATCH 1/2] Enable Intel AVX512_FP16
>>>>>> instructions
>>>>>>
>>>>>> On 13.07.2021 08:58, Cui, Lili wrote:
>>>>>>
>>>>>> Disassembler:
>>>>>>
>>>>>> d_scalar_mode looks to be unused.
>>>>>>
>>>>>> This
>>>>>>
>>>>>>   /* EVEX_W_MAP5_2A_P_1 */
>>>>>>   {
>>>>>>     { "vcvtsi2sh{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR, Ed }, 0 },
>>>>>>     { "vcvtsi2sh{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR, Eq }, 0 },
>>>>>>   },
>>>>>>
>>>>>> can imo be expressed without decoding EVEX.W, by using Edq instead
>>>>>> of
>>>>>> (separately) Ed and Eq. There's at least one similar case elsewhere.
>>>>>> Interestingly in the 2si/2usi conversions you do use Gdq already,
>>>>>> which I think handles the EVEX.W=1 case correctly outside of 64-bit
>>>>>> mode (unlike Eq, which will unconditionally produce 64-bit register
>>>>>> names
>>>> afaict).
>>>>>>
>>>>>> As to a broader question on decoding EVEX.W: Did you consider
>>>>>> introducing e.g. %XH (paralleling %XW, just that EVEX.W=1 is not a
>>>>>> valid encoding), to avoid this decode step for perhaps almost all
>>>>>> entries? And if that's not an option, decoding EVEX.W first for all
>>>>>> the opcodes which previously had no meaning at all would, in some
>>>>>> cases, reduce the overall number of table entries (and in all other
>>>>>> cases this would then merely be for consistency, as it also
>>>>>> wouldn't
>>>> increase the number of table entries). To give an example:
>>>>>>
>>>>>>     { PREFIX_TABLE (PREFIX_EVEX_0F3AC2) },
>>>>>>
>>>>>> =>
>>>>>>
>>>>>>   /* PREFIX_EVEX_0F3AC2 */
>>>>>>   {
>>>>>>     { VEX_W_TABLE (EVEX_W_0F3AC2_P_0) },
>>>>>>     { VEX_W_TABLE (EVEX_W_0F3AC2_P_1) },
>>>>>>   },
>>>>>>
>>>>>> =>
>>>>>>
>>>>>>   /* EVEX_W_0F3AC2_P_0 */
>>>>>>   {
>>>>>>     { "vcmpph",	{ XMask, Vex, EXxh, EXxEVexS, Ib }, 0 },
>>>>>>   },
>>>>>>   /* EVEX_W_0F3AC2_P_1 */
>>>>>>   {
>>>>>>     { "vcmpsh",	{ XMask, VexScalar, EXxmm_mw, EXxEVexS, Ib }, 0 },
>>>>>>   },
>>>>>>
>>>>>> i.e. a total of 1 + 4 + 2 * 2 entries. Whereas decoding W first
>>>>>> would yield 1
>>>>>> (evex) + 2 (evex_w) + 4 (prefix) entries.
>>>>>
>>>>> Hi Jan,
>>>>>
>>>>> Do you want me to change it like this?
>>>>>      { PREFIX_TABLE (PREFIX_EVEX_0F3AC2) },
>>>>>
>>>>>  =>
>>>>>
>>>>>    /* PREFIX_EVEX_0F3AC2 */
>>>>>    {
>>>>>      { "vcmp%XH",	{ XMask, Vex, EXxh, EXxEVexS, Ib }, 0 },
>>>>>      { "vcmp%XH",	{ XMask, VexScalar, EXxmm_mw, EXxEVexS, Ib }, 0 },
>>>>>    },
>>>>>
>>>>> "XH" => print 'ph', 'sh' depending on the EVEX.ll bit, if EVEX.W==W1
>>>>> report
>>>> bad code.
>>>>> if  (EVEX.LL== EVEX.LLIG)
>>>>>       print 'sh'
>>>>> else
>>>>>       print 'ph'
>>>>
>>>> Not exactly, no. %XH was meant to parallel %XW, which prints 's' or 'd'
>>>> depending on VEX.W. %XH would print 'h' if EVEX.W is clear and
>>>> produce an appropriate indication of the encoding being bad if EVEX.W is
>> set.
>>>> IOW something like
>>>>
>>>>    /* PREFIX_EVEX_0F3AC2 */
>>>>    {
>>>>      { "vcmpp%XH",	{ XMask, Vex, EXxh, EXxEVexS, Ib }, 0 },
>>>>      { "vcmps%XH",	{ XMask, VexScalar, EXxmm_mw, EXxEVexS, Ib }, 0 },
>>>>    },
>>>>
>>>>>> The delta is even larger for something like MAP5_7D: 1 + 4 + 4 * 2
>>>>>> vs. 1 + 2 + 4. This also results in more related entries ending up
>>>>>> closer to one another.
>>>>>>
>>>>> I don't quite understand here,  should I let all FP16 disassembler
>>>>> go
>>>> through W_TABLE fist? or just add something like %XH instead of going
>>>> through W_TABLE? Thanks.
>>>>
>>>> Where beneficial you will want to decode EVEX.W first, yes. Unless,
>>>> as per above, you can avoid that decoding step altogether by using %XH.
>>>>
>>> I prefer to decode EVEX.W first instead of using %XH.
>>
>> Well, I can't stop you from avoiding %XH, but I did intentionally say "Where
>> beneficial you will want to ...". That is, I think that where possible you should
>> use %XH, and only where that's not suitable decode EVEX.W (typically earlier
>> than EVEX.pp).
>>
> OK, for the instruction that EVEX.W cannot be decoded earlier than EVEX.PP, I will use %XH.

I'm sorry, but no, it's the other way around. The first check would be whether
%XH can be used. Only then would you check whether decoding EVEX.W first is at
least no worse than decoding EVEX.pp first; I think there will be few if any
cases where decoding EVEX.pp first is beneficial.

Jan