On Thu, Jul 18, 2024 at 9:29 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Thu, Jul 18, 2024 at 03:23:05PM +0800, MayShao-oc wrote:
> > From: mayshao <mayshao...@zhaoxin.com>
> >
> > Hi Jakub:
> >
> > Thanks for your review,We should just amend this to handle Zhaoxin.
> >
> > Bootstrapped /regtested X86_64.
> >
> > Ok for trunk?
> > BR
> > Mayshao
> >
> > libatomic/ChangeLog:
> >
> > PR target/104688
> > * config/x86/init.c (__libat_feat1_init): Don't clear
> > bit_AVX on ZHAOXIN CPUs.
> > ---
> > libatomic/config/x86/init.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/libatomic/config/x86/init.c b/libatomic/config/x86/init.c
> > index a75be3f175c..0d6864909bb 100644
> > --- a/libatomic/config/x86/init.c
> > +++ b/libatomic/config/x86/init.c
> > @@ -39,12 +39,15 @@ __libat_feat1_init (void)
> > == (bit_AVX | bit_CMPXCHG16B))
> > {
> > /* Intel SDM guarantees that 16-byte VMOVDQA on 16-byte aligned
> > address
> > - is atomic, and AMD is going to do something similar soon.
> > - We don't have a guarantee from vendors of other CPUs with AVX,
> > - like Zhaoxin and VIA. */
> > - unsigned int ecx2 = 0;
> > + is atomic, and AMD is going to do something similar soon. Zhaoxin
> > also
>
> Two spaces before Zhaoxin (and also should go on another line).
>
> > + guarantees this. We don't have a guarantee from vendors of other CPUs
>
> Two spaces before We (and again, the line will be too long).
>
> > + with AVX,like VIA. */
>
> Space before like
>
> > + unsigned int ecx2 = 0, family = 0;
No need to initialize these two variables. Please also add one line of
vertical space after variable declarations.
OK with the above change and with Jakub's proposed formatting changes.
Thanks,
Uros.
> > + family = (eax >> 8) & 0x0f;
> > __get_cpuid (0, &eax, &ebx, &ecx2, &edx);
> > - if (ecx2 != signature_INTEL_ecx && ecx2 != signature_AMD_ecx)
> > + if (ecx2 != signature_INTEL_ecx && ecx2 != signature_AMD_ecx
>
> If the whole condition can't fit on one line, then each subcondition should
> be on a separate line, so linebreak + indentation should be added also
> before && ecx2 != signature_AMD_ecx
>
> > + && !(ecx2 == signature_CENTAUR_ecx && family > 0x6)
> > + && ecx2 != signature_SHANGHAI_ecx)
> > FEAT1_REGISTER &= ~bit_AVX;
> > }
> > #endif
> > --
> > 2.27.0
>
> Otherwise LGTM, but please give Uros a day or two to chime in.
>
> Jakub
>
