Re: [dpdk-dev] [PATCH v2] build: disable compiler AVX512F support

["Wiles, Keith" <keith.wiles@intel.com>]

> On Nov 6, 2018, at 9:30 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> 
> 
>> On Nov 5, 2018, at 6:06 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
>> 
>> 
>> 
>>> On Nov 2, 2018, at 9:04 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
>>> 
>>> This is a workaround to prevent a crash, which might be caused by
>>> optimization of newer gcc (7.3.0) on Intel Skylake.
>> 
>> Should the code below not also test for the gcc version and the Sky Lake processor, maybe I am wrong but it seems it is turning AVX512 for all GCC builds
> 
> I didn't want to check gcc version as 7.3.0 is very new. Only gcc 8 is newly up since then (gcc 8.2).
> Also, I wasn't able to test every gcc versions and I wanted to be a bit conservative for this crash.
> Performance drop (if any) by disabling a new (experimental) feature would be less risky than unaccountable crash.
> And, it does disable the feature only if CONFIG_RTE_ENABLE_AVX512=n. Please refer to v3.

Are you not turning off all of the GCC versions for AVX512. And you can test for range or greater then GCC version and it just seems like we are turning off every gcc version, is that true?
> 
>> 
>> Also bug 97 seems a bit obscure reference, maybe you know the bug report, but more details would be good?
> 
> I sent out the report to dev list two month ago. And I created the Bug 97 in order to reference it in the commit message.
> I didn't want to repeat same message here and there, but it would've been better to have some sort of summary of the Bug, although v3 has a few more words. However, v3 has been merged.

Still this is too obscure if nothing else give a link to a specific bug not just 97.
> 
> Thanks,
> Yongseok
> 
>>> 
>>> Bugzilla ID: 97
>>> 
>>> Cc: stable@dpdk.org
>>> 
>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>> ---
>>> 
>>> v2:
>>> * disable the flag only in case of gcc
>>> 
>>> config/x86/meson.build | 5 +++++
>>> mk/rte.cpuflags.mk     | 7 +++++++
>>> 2 files changed, 12 insertions(+)
>>> 
>>> diff --git a/config/x86/meson.build b/config/x86/meson.build
>>> index 33efb5e547..8ddca0ea9f 100644
>>> --- a/config/x86/meson.build
>>> +++ b/config/x86/meson.build
>>> @@ -47,6 +47,11 @@ endif
>>> if cc.get_define('__AVX512F__', args: march_opt) != ''
>>> 	dpdk_conf.set('RTE_MACHINE_CPUFLAG_AVX512F', 1)
>>> 	compile_time_cpuflags += ['RTE_CPUFLAG_AVX512F']
>>> +else
>>> +# disable AVX512F support of gcc as a workaround for Bug 97
>>> +	if cc.get_id() == 'gcc' and cc.has_argument('-mavx512f')
>>> +		machine_args += '-mno-avx512f'
>>> +	endif
>>> endif
>>> 
>>> dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
>>> diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk
>>> index 43ed84155b..a8c26fb011 100644
>>> --- a/mk/rte.cpuflags.mk
>>> +++ b/mk/rte.cpuflags.mk
>>> @@ -68,6 +68,13 @@ endif
>>> ifneq ($(filter $(AUTO_CPUFLAGS),__AVX512F__),)
>>> ifeq ($(CONFIG_RTE_ENABLE_AVX512),y)
>>> CPUFLAGS += AVX512F
>>> +else
>>> +# disable AVX512F support of gcc as a workaround for Bug 97
>>> +ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
>>> +	ifeq ($(shell $(CC) --target-help | grep -q mavx512f && echo 1), 1)
>>> +		MACHINE_CFLAGS += -mno-avx512f
>>> +	endif
>>> +endif
>>> endif
>>> endif
>>> 
>>> -- 
>>> 2.11.0
>>> 
>> 
>> Regards,
>> Keith
> 

Regards,
Keith