build: disable compiler AVX512F support
Checks
Commit Message
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
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(+)
Comments
23/10/2018 23:23, Yongseok Koh:
> 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
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
I was asked to wait before applying this patch.
10 days passed and there is no comment, so I will apply it tomorrow.
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.
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.
> +MACHINE_CFLAGS += -mno-avx512f
> +endif
> endif
> endif
>
>
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.
>
>> +MACHINE_CFLAGS += -mno-avx512f
>> +endif
>> endif
>> endif
>>
>>
>
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.
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
> >>
> >>
> >
>
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
>>>>
>>>>
>>>
>>
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
> >>>>
> >>>>
> >>>
> >>
>
@@ -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)
@@ -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)
+MACHINE_CFLAGS += -mno-avx512f
+endif
endif
endif