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: sta...@dpdk.org
>>>>
>>>> Signed-off-by: Yongseok Koh <ys...@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
>>>>
>>>>
>>>
>>
