[PATCH v5] Support ymm rounding control for Intel AVX10.2

[Jan Beulich]

On 27.08.2024 09:08, Jiang, Haochen wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, August 26, 2024 10:23 PM
>>
>> On 23.08.2024 10:38, Haochen Jiang wrote:
>>> In the patch, in order to support ymm rounding for AVX10.2, we derive
>>> evex attribute for all cases instead of only for rc_none to encode U bit.
>>> Also changed some bad_opcode return due to the share of U bit with APX_F.
>>>
>>> Instead of adding new table entries, our current method is to alter the
>>> CPUID part to add AVX10_2 explicitly for ymm rounding support, also
>>> using similar way as APX_F for the CPUID combination, since ymm rounding
>>> control is also somehow an instruction promotion, i.e.:
>>>
>>>   #define AVX10_2(cpuid) cpuid&(cpuid|AVX10_2)
>>
>> The APX attribute was only added as necessary. Legacy insns being extended
>> to use REX2 encodings weren't altered. Similarly no AVX10.1 attributes were
>> added anywhere. IOW as before - please limit the opcode table changes to
>> just those places where new insn forms need adding (I expect that's going
>> to be a couple of convert insns with differring source/destination element
>> sizes).
> 
> AVX10.1 definitely doesn't need to be added everywhere since we are actually
> implying AVX512.
> 
> However, AVX10.2 is different, it does contain new feature for ymm rounding.
> I am not changing everywhere but only altering those related instruction CPUID,
> which is the maybe optimal change for now since splitting encoding_evex512
> will be a separate patch. All of them are needed, which are actually the new insn
> forms since ymm rounding doesn't exist before AVX10.2.

Well, no, I'm afraid you're not going to convince me unless you come
forward with a hard technical requirement. Yet really it is the other
way around: Take

vcvtph2pd, 0x5a, AVX512_FP16&(AVX512VL|AVX10_2), Modrm|EVex256|Masking|EVexMap5|VexW0|Broadcast|Disp8MemShift=3|NoSuf|SAE, { RegXMM|Word|Qword|Unspecified|BaseIndex, RegYMM }

as example: What use is the AVX10_2 here? Even with AVX10.2 support
turned off, as long as AVX512VL is enabled the template is going to be
eligible for use. It is still solely the code you're adding to
check_VecOperands() which makes sure SAE cannot be used in such a case.

And for the case where one mentally considers AVX512VL disabled, the
resulting expression is actively misleading: AVX512_FP16&AVX10_2 is
wrong - AVX10_1 is sufficient for this insn to be used, as that (and
not AVX10.2) implies AVX512-FP16 (except of course for the possible
vector length restriction).

You need to touch this template, yes, but only to add SAE.

>>> +  u = (i.rounding.type != rc_none
>>> +       && i.tm.opcode_modifier.evex == EVEX256) ? false : true;
>>
>> What's the conditional operator for? This
>>
>>   u = (i.rounding.type == rc_none
>>        || i.tm.opcode_modifier.evex != EVEX256);
>>
>> is imo easier to follow.
> 
> It comes from original 0-1. Yes, it should be that after changing to bool.

Just to mention it: When u still wasn't bool, no conditional operator
should have needed using either. I know there are examples to the contrary
in existing code - I suppose all of them should be changed.

>>> +	 enabled.  There's no need to check all operands, though: Either of the
>>>  	 last two operands will be of the right size in all relevant templates.  */
>>>        if (t->opcode_modifier.evex != EVEXLIG
>>>  	  && t->opcode_modifier.evex != EVEX512
>>> +	  && (t->opcode_modifier.evex != EVEX256
>>> +	      || !cpu_arch_flags.bitfield.cpuavx10_2)
>>>  	  && !i.types[t->operands - 1].bitfield.zmmword
>>> -	  && !i.types[t->operands - 2].bitfield.zmmword)
>>> +	  && !i.types[t->operands - 2].bitfield.zmmword
>>> +	  && ((!i.types[t->operands - 1].bitfield.ymmword
>>> +	       && !i.types[t->operands - 2].bitfield.ymmword)
>>> +	      || !cpu_arch_flags.bitfield.cpuavx10_2))
>>>  	{
>>>  	  i.error = operand_size_mismatch;
>>>  	  return 1;
>>
>> Since you now uniformly resolve t->opcode_modifier.evex when it's EVEXDYN,
>> do you actually still need the .zmmword / .ymmword checks? In the
>> calculation of the U bit you also rely on solely that attribute.
> 
> check_VecOperands() comes earlier than build_evex_prefix(). At this point,
> the evex for EVEXDYN has not been derived. We could and I have considered to
> move that part into check_VecOperands(), but I suppose it might be out of
> scope for this function.

Oh, I'm sorry. The sequence of hunks misled me. You're right, and I agree
with not moving that logic.

Jan