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

[Yongseok Koh <yskoh@mellanox.com>]

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: 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`?

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
> >>>>  
> >>>>
> >>>
> >>
>