On Fri, Nov 02, 2018 at 09:46:09PM +0000, Ferruh Yigit wrote:
> 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`?
Right. For some reason, I have wrong impression that the flag grabs cpuflags of
the compile host. Will submit a new version.
> > 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
> >>>>
> >>>>
> >>>
> >>
>
