| Message ID | 20240620130254.415699-7-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | host/i386: allow configuring the x86-64 baseline | expand |
On Thu, Jun 20, 2024 at 03:02:54PM +0200, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > meson.build | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/meson.build b/meson.build > index 54e6b09f4fb..c5360fbd299 100644 > --- a/meson.build > +++ b/meson.build > @@ -2863,6 +2863,7 @@ have_cpuid_h = cc.links(''' > config_host_data.set('CONFIG_CPUID_H', have_cpuid_h) > > config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ > + .enable_auto_if(get_option('x86_version') >= '3') \ > .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX2') \ > .require(cc.links(''' > #include <cpuid.h> > @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ > '''), error_message: 'AVX2 not available').allowed()) > > config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \ > + .enable_auto_if(get_option('x86_version') >= '4') \ > .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX512BW') \ > .require(cc.links(''' > #include <cpuid.h> I'm not sure this makes sense. The CONFIG_AVX* options are used only to validate whether the toolchain has support for this. The QEMU code then has a runtime, so it automagically uses AVX2/AVX512 if-and-only-if running on a suitably new CPU. IOW, we want this enabled always when the toolchain supports it, regardless of what x86_version is set. With regards, Daniel
On Thu, Jun 20, 2024 at 5:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ > > + .enable_auto_if(get_option('x86_version') >= '3') \ > > .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX2') \ > > .require(cc.links(''' > > #include <cpuid.h> > > @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ > > '''), error_message: 'AVX2 not available').allowed()) > > > > config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \ > > + .enable_auto_if(get_option('x86_version') >= '4') \ > > .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX512BW') \ > > .require(cc.links(''' > > #include <cpuid.h> > > I'm not sure this makes sense. The CONFIG_AVX* options are used only > to validate whether the toolchain has support for this. The QEMU > code then has a runtime, so it automagically uses AVX2/AVX512 > if-and-only-if running on a suitably new CPU. IOW, we want this > enabled always when the toolchain supports it, regardless of what > x86_version is set. The difference is that if the toolchain does not support AVX2/AVX512 intrinsics for some reason, and you require -Dx86_version={3,4}, meson would report an error with this patch. Paolo
On 6/20/24 08:01, Daniel P. Berrangé wrote: > On Thu, Jun 20, 2024 at 03:02:54PM +0200, Paolo Bonzini wrote: >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> meson.build | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/meson.build b/meson.build >> index 54e6b09f4fb..c5360fbd299 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -2863,6 +2863,7 @@ have_cpuid_h = cc.links(''' >> config_host_data.set('CONFIG_CPUID_H', have_cpuid_h) >> >> config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ >> + .enable_auto_if(get_option('x86_version') >= '3') \ >> .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX2') \ >> .require(cc.links(''' >> #include <cpuid.h> >> @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ >> '''), error_message: 'AVX2 not available').allowed()) >> >> config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \ >> + .enable_auto_if(get_option('x86_version') >= '4') \ >> .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX512BW') \ >> .require(cc.links(''' >> #include <cpuid.h> > > I'm not sure this makes sense. The CONFIG_AVX* options are used only > to validate whether the toolchain has support for this. The QEMU > code then has a runtime, so it automagically uses AVX2/AVX512 > if-and-only-if running on a suitably new CPU. IOW, we want this > enabled always when the toolchain supports it, regardless of what > x86_version is set. Indeed. To me this means they should not be configure options at all. We should simply detect compiler support, end of. r~
On Thu, Jun 20, 2024 at 7:22 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > I'm not sure this makes sense. The CONFIG_AVX* options are used only > > to validate whether the toolchain has support for this. The QEMU > > code then has a runtime, so it automagically uses AVX2/AVX512 > > if-and-only-if running on a suitably new CPU. IOW, we want this > > enabled always when the toolchain supports it, regardless of what > > x86_version is set. > > Indeed. To me this means they should not be configure options at all. > We should simply detect compiler support, end of. Ok, I'll drop this patch then for now. Paolo
diff --git a/meson.build b/meson.build index 54e6b09f4fb..c5360fbd299 100644 --- a/meson.build +++ b/meson.build @@ -2863,6 +2863,7 @@ have_cpuid_h = cc.links(''' config_host_data.set('CONFIG_CPUID_H', have_cpuid_h) config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ + .enable_auto_if(get_option('x86_version') >= '3') \ .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX2') \ .require(cc.links(''' #include <cpuid.h> @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \ '''), error_message: 'AVX2 not available').allowed()) config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \ + .enable_auto_if(get_option('x86_version') >= '4') \ .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable AVX512BW') \ .require(cc.links(''' #include <cpuid.h>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- meson.build | 2 ++ 1 file changed, 2 insertions(+)