On Thu, Jul 18, 2024 at 10:21 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Thu, Jul 18, 2024 at 10:12:46AM +0200, Uros Bizjak wrote:
> > On Thu, Jul 18, 2024 at 9:50 AM Jakub Jelinek <ja...@redhat.com> wrote:
> > >
> > > On Thu, Jul 18, 2024 at 09:34:14AM +0200, Uros Bizjak wrote:
> > > > > > + unsigned int ecx2 = 0, family = 0;
> > > >
> > > > No need to initialize these two variables.
> > >
> > > The function ignores __get_cpuid result, so at least the
> > > FEAT1_REGISTER = 0; is needed before the first __get_cpuid.
> > > Do you mean the ecx2 = 0 initialization is useless because
> > > __get_cpuid (0, ...) on x86_64 will always succeed (especially
> > > when __get_cpuid (1, ...) had to succeed otherwise FEAT1_REGISTER
> > > would be zero)?
> > > I guess that is true, but won't that cause -Wmaybe-uninitialized warnings?
> >
> > Yes, if the __get_cpuid (1, ...) works OK, then we are sure that
> > __get_cpuid (0, ...) will also work.
> >
> > > I agree initializing family to 0 is not needed, but I don't understand
> > > why it isn't just
> > > unsigned family = (eax >> 8) & 0x0f;
> > > Though, guess even that might fail with -Wmaybe-uninitialized too, as
> > > eax isn't unconditionally initialized.
> >
> > Perhaps we should check the result of __get_cpuid (1, ...) and use eax
> > only if the function returns 1? IMO, this would solve the
> > uninitialized issue, and we could use __cpuid in the second case (we
> > would know that leaf 0 is supported, because leaf 1 support was
> > checked with __get_cpuid (1, ...)).
>
> We know the code is ok if FEAT1_REGISTER = 0; is done before __get_cpuid (1,
> ...).
> Everything else is implied from it, all we need to ensure is that
> -Wmaybe-uninitialized is happy about it.
> Whatever doesn't report the warning and ideally doesn't increase the size of
> the function.
> I think the reason it is written the way it is before the AVX hacks in it
> is that we need to handle even the case when __get_cpuid (1, ...) returns 0,
> and we want in that case FEAT1_REGISTER = 0.
> So it could be
Yes, I think this is better, see below.
> FEAT1_REGISTER = 0;
> #ifdef __x86_64__
> if (__get_cpuid (1, &eax, &ebx, &ecx, &edx)
> && (FEAT1_REGISTER & (bit_AVX | bit_CMPXCHG16B))
> == (bit_AVX | bit_CMPXCHG16B))
> {
Here we can simply use
unsigned int family = (eax >> 8) & 0x0f;
unsigned int ecx2;
__cpuid (0, eax, ebx, ecx2, edx);
if (ecx2 ...)
> ...
> }
> #else
> __get_cpuid (1, &eax, &ebx, &ecx, &edx);
> #endif
> etc.
>
> Jakub
Uros.
