[PATCH] Support ymm rounding control for Intel AVX10.2
Jiang, Haochen
haochen.jiang@intel.com
Fri Aug 2 02:39:33 GMT 2024
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, August 1, 2024 6:08 PM
> To: Jiang, Haochen <haochen.jiang@intel.com>
> Cc: hjl.tools@gmail.com; binutils@sourceware.org
> Subject: Re: [PATCH] Support ymm rounding control for Intel AVX10.2
>
> On 01.08.2024 09:33, Haochen Jiang wrote:
> > --- a/gas/NEWS
> > +++ b/gas/NEWS
> > @@ -1,5 +1,7 @@
> > -*- text -*-
> >
> > +* Add support for Intel AVX10.2 instructions.
> > +
>
> No, I'm sorry. This is premature.
I will move this to the end of the patch series. I mean, when all the
new insts are also completed.
>
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -402,6 +402,8 @@ struct _i386_insn
> > /* In Intel syntax the operand modifier form is supposed to be used, but
> > we continue to accept the immediate forms as well. */
> > bool modifier;
> > + /* The operand is ymm or not. */
> > + bool ymm;
> > } rounding;
> >
> > /* Broadcasting attributes.
> > @@ -1186,6 +1188,7 @@ static const arch_entry cpu_arch[] =
> > VECARCH (avx10.1, AVX10_1, ANY_AVX512F, set),
> > SUBARCH (user_msr, USER_MSR, USER_MSR, false),
> > SUBARCH (apx_f, APX_F, APX_F, false),
> > + VECARCH (avx10.2, AVX10_2, ANY_AVX10_2, set),
> > };
> >
> > #undef SUBARCH
> > @@ -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.
>
> > @@ -4205,7 +4209,7 @@ get_broadcast_bytes (const insn_template *t,
> > bool diag) static void build_evex_prefix (void) {
> > - unsigned int register_specifier, w;
> > + unsigned int register_specifier, w, u;
> > rex_byte vrex_used = 0;
> >
> > /* Check register specifier. */
> > @@ -4278,10 +4282,12 @@ build_evex_prefix (void)
> > else
> > w = (flag_code == CODE_64BIT ? i.rex & REX_W : evexwig == evexw1)
> > ? 1 : 0;
> >
> > + u = (i.rounding.type != rc_none && i.rounding.ymm) ? 0 : 1;
>
> Do you really need the i.rounding.ymm bit here? Can't you go from what
> encodes as EVEX.L'L, i.e. the EVex attribute?
I suppose the bit will reduce the understanding cost. But let me see if we
can get something a little complicated in logic to remove this bit and see
the trade-offs.
>
> > --- /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.
This is also applicable for all previous testcases currently in Binutils.
>
> > +#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?
>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/avx10_2-rounding.s
> > @@ -0,0 +1,888 @@
> > +# Check 32bit AVX10.2 instructions
> > +
> > + .text
> > +_start:
> >[...]
> > + .intel_syntax noprefix
> > + vaddpd ymm6, ymm5, ymm4, {rn-sae}
>
> New tests want to please use the proper Intel syntax form; the forms with the
> rounding/SAE specifier being a separate operand are supported only for
> backwards compatibility. See also how the above does not match what the
> disassembler produces.
>
> Furthermore when adding new large tests with lots of redundancy, please at
> least consider macro-izing a little. E.g. at least the FMA 132, 213, and
> 231 forms, which make for a substantial amount of the redundancy.
Let me have a try on that.
>
> Testcase comments also apply to the 64-bit testcases then.
>
> > --- 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.
Let me double check the logic for REX_B.
>
> > @@ -9088,14 +9091,20 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> > if (!fetch_modrm (ins))
> > return &err_opcode;
> >
> > - if (ins->modrm.mod == 3 && (ins->rex2 & REX_X))
> > + /* When modrm.mod != 3, the U bit is used by APX for bit X4.
> > + When modrm.mod == 3, the U bit is used by AVX10. The U bit and
> > + the b bit should not be zero at the same time. */
> > + if (ins->modrm.mod == 3 && !ins->vex.u && !ins->vex.b)
> > return &bad_opcode;
>
> Doesn't this further want/need qualifying by an ins->evex_type check?
I get your concern, I will have a look if changing this will make some invalid
bytecode valid.
>
> > /* Set vector length. For EVEX-promoted instructions, evex.ll == 0b00,
> > which has the same encoding as vex.length == 128 and they can share
> > the same processing with vex.length in OP_VEX. */
> > if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type !=
> evex_from_legacy)
> > - ins->vex.length = 512;
> > + if (ins->vex.u)
> > + ins->vex.length = 512;
> > + else
> > + ins->vex.length = 256;
>
> Such inner if()/else want parenthesizing against the surrounding if()...
I will change that, it will be potential buggy without parenthesizing when
some other change is applied here.
>
> > else
> > {
> > switch (ins->vex.ll)
>
> ... /else.
>
> > --- a/opcodes/i386-opc.h
> > +++ b/opcodes/i386-opc.h
> > @@ -321,6 +321,8 @@ enum i386_cpu
> > CpuAVX512VL,
> > /* Intel APX_F Instructions support required. */
> > CpuAPX_F,
> > + /* Intel AVX10.2 Instructions support required. */ CpuAVX10_2,
> > /* Not supported in the 64bit mode */
> > CpuNo64,
> >
> > @@ -357,6 +359,7 @@ enum i386_cpu
> > cpuavx512f:1, \
> > cpuavx512vl:1, \
> > cpuapx_f:1, \
> > + cpuavx10_2:1, \
> > /* NOTE: This field needs to remain last. */ \
> > cpuno64:1
>
> Why are you adding the enum/bit like this? The new flag is used on its own
> only afaics, and hence doesn't need to be in the set of ones permitting
> combination with other features. The idea of the splitting was that adding
> support for "simple" features like this one would not require the number of
> bitfield elements to grow.
>
> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> > @@ -2908,12 +2908,12 @@ vptestnm<bw>, 0xf326, AVX512BW,
> > Modrm|Masking|Space0F38|Src1VVVV|<bw:vexw>|Disp8
> >
> > // AVX512DQ instructions.
> >
> > -<xyz:vl:attr:sr:att:src, +
> > -
> $i::Disp8ShiftVL|IntelSyntax:StaticRounding|SAE::RegXMM|RegYMM|RegZM
> M|Unspecified|BaseIndex, +
> > -
> $a::Disp8ShiftVL|ATTSyntax:StaticRounding|SAE::RegXMM|RegYMM|RegZM
> M|BaseIndex, +
> > -
> z::EVex512|Disp8MemShift=6:StaticRounding|SAE:ATTSyntax:RegZMM|Uns
> pecified|BaseIndex, +
> > -
> x:AVX512VL:EVex128|Disp8MemShift=4::ATTSyntax:RegXMM|Unspecified|B
> aseIndex, +
> > -
> y:AVX512VL:EVex256|Disp8MemShift=5::ATTSyntax:RegYMM|Unspecified|B
> aseIndex>
> > +<xyz:vl:evex:attr:sr:att:src, +
> > +
> $i:::Disp8ShiftVL|IntelSyntax:StaticRounding|SAE::RegXMM|RegYMM|RegZ
> MM|Unspecified|BaseIndex, +
> > +
> $a:::Disp8ShiftVL|ATTSyntax:StaticRounding|SAE::RegXMM|RegYMM|RegZ
> MM|BaseIndex, +
> > +
> z::EVex512:EVex512|Disp8MemShift=6:StaticRounding|SAE:ATTSyntax:RegZ
> MM|Unspecified|BaseIndex, +
> > +
> x:AVX512VL:EVex128:EVex128|Disp8MemShift=4::ATTSyntax:RegXMM|Uns
> pecified|BaseIndex, +
> > +
> >
> +y:AVX512VL:EVex256:EVex256|Disp8MemShift=5::ATTSyntax:RegYMM|Uns
> pecif
> > +ied|BaseIndex>
>
> What's the reason for this change? I can't spot any use of the new <xyz:evex>.
Ah... It is being used and unused when patch evolves but unused at the end of
the day. I will remove them.
>
> > @@ -3575,3 +3575,96 @@ pop2, 0x8f/0, APX_F,
> > Modrm|VexW0|EVexMap4|DstVVVV|ImplicitStackOp|No_bSuf|No_wSu
> > pop2p, 0x8f/0, APX_F,
> >
> Modrm|VexW1|EVexMap4|DstVVVV|ImplicitStackOp|No_bSuf|No_wSuf|N
> o_lSuf|N
> > o_sSuf, { Reg64, Reg64 }
> >
> > // APX Push2/Pop2 instructions end.
> > +
> > +// AVX10.2 instructions.
> > +
> > +<yr:att:sr:sae:src, +
> > +
> $i:IntelSyntax:StaticRounding|SAE:SAE:RegYMM|Unspecified|BaseIndex, +
> > + $a:ATTSyntax:StaticRounding|SAE:SAE:RegYMM|BaseIndex, +
> > + y:ATTSyntax:StaticRounding|SAE:SAE:RegYMM|Unspecified|BaseIndex>
>
> I think Ey would be a better name for this, even if I can guess what yr is meant
> to stand for. Neither the sr nor the sae attributes should exist, though.
> They're both identical between all three variants.
>
> Thinking about it: Is this needed at all? The variants exist because of the
> memory operands, where the sizes need expressing in some way. Yet this
> patch is solely about register-only forms, where sizes are known from
> operands.
You get a point here. I will remove all memory, they should not be needed at all.
>
> > +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.
Thx,
Haochen
> to every template permitting SAE? Exceptions may be the ones where you end
> up using <yr> right now, but quite possible even those can be covered.
>
> Jan
More information about the Binutils
mailing list