[PATCH 1/2] [PATCH 1/2] Enable Intel AVX512_FP16 instructions

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, July 21, 2021 10:29 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 21.07.2021 15:14, Cui, Lili wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, July 14, 2021 12:25 AM On 13.07.2021 08:58, Cui,
> >> Lili wrote:
> >>
> >> I don't think you've applied my comments regarding the
> >> cpu_flag_init[] additions
> >
> > Sorry, I forgot this one, I changed it in this patch.
> >
> > { "CPU_AVX512_FP16_FLAGS",
> >     "CPU_AVX512BW_FLAGS|CpuAVX512_FP16" }, ...
> >   { "CPU_ANY_AVX512BW_FLAGS",
> >     "CpuAVX512BW|CpuAVX512_FP16" },
> > ...
> 
> In the latter case you will want to use CPU_ANY_AVX512_FP16_FLAGS, not
> CpuAVX512_FP16. This not originally having got done properly for
> CPU_ANY_AVX512F_FLAGS is why you now also need to change that one.
> There preferably you'd replace CpuAVX512BW by
> CPU_ANY_AVX512BW_FLAGS, instead of explicitly adding CpuAVX512_FP16.
> 
Done.
 
> Ah yes, albeit there's some confusion about the numbering of the attached
> patches. I wanted to ask you anyway to send new versions of the patches as
> new mail threads, instead of in reply to the prior discussion. In fact, while the
> patch with the new tests is probably indeed too large to send inline (but as
> said before, it's not really reviewable anyway), I would much appreciate if
> you could send the first patch inline instead of as attachment. This makes
> commenting a lot easier.
> 
Sorry, I thought you would feel too big to put source patch inline, anyway I will let them inline next.

> As a purely cosmetic request - may I ask that you flip the order of the two
> each vcmp<avx_frel>ph and vcmpph entries, such that - like everywhere else
> - the rounding/SAE forms come second? As mentioned previously, keeping
> things as consistent as possible helps readers as well as making future
> changes or spotting possible problems.
> 
Done.

> As to the testsuite additions, I'm afraid you've misunderstood what H.J. and I
> are requesting for xmmword.{s,d}: You don't need to add _all_ new insns
> there - that would be making an unreasonably large test case going forward.
> Only insns with irregular operand combinations permitting %xmmN but not
> "xmmword ptr ..." for a given operand position need putting there. This is to
> prove (now and going
> forward) that despite allowing %xmmN the (wrong size) memory form gets
> rejected. Regular scalar insns don't need putting there, and even less so
> VDIVPH; if at all, a single random example (not from the FP16 set, but from
> more basic ones) would be sufficient. So what I was after with my request
> was that all the non-scalar VCVT* forms would be represented there, when
> their vector element sizes vary.
> 
Done

> I'm sorry if the earlier request wasn't explicit enough.
> 
No, that was my fault. Thanks again for your patience.

> The other remark on the testsuite addition patch is that _if_ you maintain a
> ChangeLog entry (which you aren't required to anymore), then you will want
> to keep it up-to-date with patch contents.
> 
I updated the ChangeLog of PATCH 2/2.

> I'll try to get to look at the assembler and disassembler parts in more detail
> later this week, as the following week I'll be OoO.

Thank you for spending so much time helping review this big patch, and hope you have a good vacation.

Lili.