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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/2/2018 8:59 PM, Yongseok Koh wrote:
> On Fri, Nov 02, 2018 at 01:48:11PM +0000, Ferruh Yigit wrote:
>> On 11/2/2018 12:42 PM, Ferruh Yigit wrote:
>>> On 10/23/2018 10:23 PM, Yongseok Koh wrote:
>>>> This is a workaround to prevent a crash, which might be caused by
>>>> optimization of newer gcc (7.3.0) on Intel Skylake.
>>>>
>>>> Bugzilla ID: 97
>>>
>>> After checking the defect description again, this is the issue observed in
>>> rte_memcpy() implementation for AVX2, compiler uses AVX512F instructions while
>>> compiling it which causes the failure, so this may be a compiler defect but we
>>> don't know the root cause yet.
>>
>> Is the issue only with gcc, and only with specific version of gcc?
>> If so can we reduce the disabling avx512 only to that gcc version?
>>
>>>
>>> I think best solution is to find the root cause and fix either avx2
>>> implementation or compiler, but this seems won't be soon, at least for rc2.
>>>
>>> What this patch does is to prevent compiler to use avx512f instruction when
>>> "CONFIG_RTE_ENABLE_AVX512=n".
>>>
>>> Concern is this will affect all DPDK generated code for x86, but since
>>> rte_memcpy() in header file there is no way to disable using avx512f
>>> instructions locally for rte_memcpy().
>>> I can't think of any other solution for now, so OK to go with this patch for
>>> now. Please find below comment.
>>>
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>>> ---
>>>>  config/x86/meson.build | 5 +++++
>>>>  mk/rte.cpuflags.mk     | 5 +++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/config/x86/meson.build b/config/x86/meson.build
>>>> index 33efb5e547..e10ba872ac 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 compiler's AVX512F support as a workaround for Bug 97
>>>> +	if 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..8fdb0cc2c3 100644
>>>> --- a/mk/rte.cpuflags.mk
>>>> +++ b/mk/rte.cpuflags.mk
>>>> @@ -68,6 +68,11 @@ endif
>>>>  ifneq ($(filter $(AUTO_CPUFLAGS),__AVX512F__),)
>>>>  ifeq ($(CONFIG_RTE_ENABLE_AVX512),y)
>>>>  CPUFLAGS += AVX512F
>>>> +else
>>>> +# disable compiler's AVX512F support as a workaround for Bug 97
>>>> +ifeq ($(shell $(CC) --target-help | grep -q mavx512f && echo 1), 1)
>>>
>>> This will not work for ICC, and do we need this? AUTO_CPUFLAGS already should
>>> have what you are looking for, so I think this check can be removed.
> 
> This is different from AUTO_CPUFLAGS as it tries to check compiler flag support.

What AUTO_CPUFLAGS does?

It is output of `cc -march=xxx -dM -E - < /dev/null`, which list defined macros
for that specific march provided.

Like if you use `-march=corei7` you won't see __AVX2__ set.

And for `native`, if compiler doesn't support AVX2, I assume it won't able to
output __AVX2__

Is there a case AUTO_CPUFLAGS has __AVX512F__ but "$(CC) --target-help" doesn't
have `mavx512f`?

> And per your question, I have only tested it with gcc, so I agree on applying it
> only for gcc. Will submit v2. But, I don't think we need to check gcc version as
> there's no fix reported yet in a newer gcc version and this patch would have
> very limited impact. avx512f support is quite new and kinda experimental so
> far. Dropping a bit of performance would be better than crash. :-)
> 
> Thanks for your review,
> Yongseok
> 
>>>> +MACHINE_CFLAGS += -mno-avx512f
>>>> +endif
>>>>  endif
>>>>  endif
>>>>  
>>>>
>>>
>>