On Thu, Jul 18, 2024 at 10:12:46AM +0200, Uros Bizjak wrote:
> On Thu, Jul 18, 2024 at 9:50 AM Jakub Jelinek <[email protected]> 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
FEAT1_REGISTER = 0;
#ifdef __x86_64__
if (__get_cpuid (1, &eax, &ebx, &ecx, &edx)
&& (FEAT1_REGISTER & (bit_AVX | bit_CMPXCHG16B))
== (bit_AVX | bit_CMPXCHG16B))
{
...
}
#else
__get_cpuid (1, &eax, &ebx, &ecx, &edx);
#endif
etc.
Jakub
