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

[Jan Beulich]

On 02.08.2024 04:39, Jiang, Haochen wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, August 1, 2024 6:08 PM
>>
>> On 01.08.2024 09:33, Haochen Jiang wrote:
>>> @@ -1732,6 +1735,7 @@ _is_cpu (const i386_cpu_attr *a, enum i386_cpu
>> cpu)
>>>      case CpuAVX512F:  return a->bitfield.cpuavx512f;
>>>      case CpuAVX512VL: return a->bitfield.cpuavx512vl;
>>>      case CpuAPX_F:    return a->bitfield.cpuapx_f;
>>> +    case CpuAVX10_2:  return a->bitfield.cpuavx10_2;
>>>      case Cpu64:       return a->bitfield.cpu64;
>>>      case CpuNo64:     return a->bitfield.cpuno64;
>>>      default:
>>
>> This shouldn't be needed; see the comment on i386-opc.h.
> 
> It seems unneeded till now or even till the end of the patch series.
> But it will be needed in the future when AVX10.2 co-operate with other ISAs
> in future processors, which should also happen in Binutils 2.44 timeframe.

We can move it when such interactions require it. It is only at that point
when it can be decided which of the features to move here.

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/avx10_2-rounding.d
>>> @@ -0,0 +1,451 @@
>>
>> This file is only half the size of avx10_2-rounding-intel.d - why?
> 
> This is because actually in those asm file, the att syntax are on the top half
> of the testcases, the second half is intel syntax. No need to check them twice
> for att syntax.
> 
> For Intel syntax, since we could not just skip the first part, the size is doubled.

Of course you can, using "#..." on a line on its own. You'll find examples
in existing testcases.

>>> +#objdump: -dw
>>> +#name: i386 AVX10.2 rounding insns
>>> +#source: avx10_2-rounding.s
>>
>> You don't really need this line here, do you?
> 
> Do you mean the empty line or some others?

I didn't comment on an empty line, but the #source one.

>>> --- a/opcodes/i386-dis.c
>>> +++ b/opcodes/i386-dis.c
>>> @@ -229,6 +229,7 @@ struct instr_info
>>>      bool b;
>>>      bool no_broadcast;
>>>      bool nf;
>>> +    bool u;
>>>    }
>>>    vex;
>>>
>>> @@ -9030,6 +9031,8 @@ get_valid_dis386 (const struct dis386 *dp,
>>> instr_info *ins)
>>>
>>>        if (!(*ins->codep & 0x4))
>>>  	ins->rex2 |= REX_X;
>>> +
>>> +      ins->vex.u = *ins->codep & 0x4;
>>>
>>>        switch ((*ins->codep & 0x3))
>>>  	{
>>> @@ -9066,7 +9069,7 @@ get_valid_dis386 (const struct dis386 *dp,
>> instr_info *ins)
>>>  	  /* Report bad for !evex_default and when two fixed values of evex
>>>  	     change..  */
>>>  	  if (ins->evex_type != evex_default
>>> -	      || (ins->rex2 & (REX_B | REX_X)))
>>> +	      && (ins->rex2 & (REX_B | REX_X)))
>>
>> I can see why you may need to change this for REX_X, but hardly for REX_B at
>> the same time?
> 
> I suppose it is a typo according to the comment, which is buggy potentially and
> found in AVX10.2.

If it was a pre-existing bug, it would want fixing separately (so it can
be backported), or at the very least it would want calling out explicitly
in the description.

>>> +vaddp<sdh>, 0x<sdh:ppfx>58, AVX10_2,
>>>
>> +Modrm|EVex256|Masking|<sdh:spc1>|Src1VVVV|<sdh:vexw>|Broadcast|
>> Disp8M
>>> +emShift=5|NoSuf|StaticRounding|SAE, {
>>> +RegYMM|<sdh:elem>|Unspecified|BaseIndex, RegYMM, RegYMM }
>>
>> At the example this: Why does this permit for a memory operand and
>> broadcast?
>> Plus it needlessly specifies Disp8MemShift. All you're after is adding the 256-
>> bit rounding form.
>>
>> In fact I'm uncertain that these new templates all need adding. Ideally no new
>> ones would be added even to the generated table, but I can see how that
>> could be challenging. But at the very least, with all of this entirely mechanical,
>> can't you instead adjust i386-gen to suitably generate AVX10.2 counterparts
> 
> I recalled that I have tried at the very beginning and did succeed, but I could not
> recall why I give up to do that and choose the current way.
> 
> Let me check that.

Thank you. In fact, after some more thinking about it, I'm now pretty sure
we can get away without adding any new "regular" templates. We may need to
add a few "irregular" ones, particularly vcvt* between vectors of different
element width.

As an aside - the fewer it is that need adding, the more desirable it will
imo be to put them next to their AVX512 counterparts, rather than in a
separate area of the file.

Jan