On 2/2/22 13:57, Stokes, Ian wrote:
>>> -----Original Message-----
>>> From: Ferriter, Cian
>>> Sent: Friday 28 January 2022 16:32
>>> To: William Tu <u9012...@gmail.com>; d...@openvswitch.org
>>> Subject: RE: [ovs-dev] [PATCH] acinclude: Detect avx512 vpopcntdq compiler
>> support.
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of William Tu
>>>> Sent: Thursday 27 January 2022 03:45
>>>> To: d...@openvswitch.org
>>>> Subject: [ovs-dev] [PATCH] acinclude: Detect avx512 vpopcntdq compiler
>> support.
>>>>
>>>> Ubuntu Xenial 16.04 is using GCC 5.4 and it does not support
>>>> target "-mavx512vpopcntdq" and cuases error
>>>> lib/dpif-netdev-lookup-avx512-gather.c:356:47:
>>>> error: attribute(target("avx512vpopcntdq")) is unknown
>>>> GCC 7+ supports vpopcntdq:
>>>> https://gcc.gnu.org/gcc-7/changes.html
>>>> The patch detects vpopcntdq and disables AVX512 when not found.
>>>>
>>>> Reported-by: Greg Rose <gvrose8...@gmail.com>
>>>> Signed-off-by: William Tu <u9012...@gmail.com>
>>>> ---
>>>> acinclude.m4 | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/acinclude.m4 b/acinclude.m4
>>>> index 5c971e98ce91..0c360fd1ef73 100644
>>>> --- a/acinclude.m4
>>>> +++ b/acinclude.m4
>>>> @@ -77,7 +77,7 @@ dnl Checks if compiler and binutils supports AVX512.
>>>> AC_DEFUN([OVS_CHECK_AVX512], [
>>>> OVS_CHECK_BINUTILS_AVX512
>>>> OVS_CHECK_CC_OPTION(
>>>> - [-mavx512f], [ovs_have_cc_mavx512f=yes],
>> [ovs_have_cc_mavx512f=no])
>>>> + [-mavx512f -mavx512vpopcntdq], [ovs_have_cc_mavx512f=yes],
>> [ovs_have_cc_mavx512f=no])
>>>> AM_CONDITIONAL([HAVE_AVX512F], [test $ovs_have_cc_mavx512f =
>> yes])
>>>> if test "$ovs_have_cc_mavx512f" = yes; then
>>>> AC_DEFINE([HAVE_AVX512F], [1],
>>>> --
>>>
>>> Hi William,
>>>
>>> Thanks for this fix, I can verify that this fixes the below error for GCC 5
>>> (it will
>> work for GCC 4.9
>>> - GCC 6):
>>> make[3]: Entering directory '/root/cian/ovs/datapath'
>>> lib/dpif-netdev-lookup-avx512-gather.c:90:1: error:
>> attribute(target("avx512vpopcntdq")) is unknown
>>>
>>> We would like to re-spin a new version of this patch that fixes the compile
>> error for the relevant GCC
>>> versions (GCC 4.9 - GCC 6) but allows some AVX512 code to be build.
>>> For setups with GCC 6 for example, we still have support for most of the
>> AVX512 ISA used in OVS, so we
>>> can still enable building of this ISA, while still correctly avoiding the
>> avx512vpopcntdq error that
>>> your patch does.
>>>
>>> Please allow us a few days to test and re-spin a new version of this patch.
>>> Thanks,
>>> Cian
>>
>> Hi again William,
>>
>> I have been looking into a new version of this patch to add partial support
>> to
>> GCC versions that don't have full support for all the AVX512 ISA we use in
>> OVS.
>> This is still a work in progress.
>>
>> While I'm looking into this more, I think it doesn't make sense to hold back
>> this
>> patch from being merged. It fixes build issues.
>>
>> I think the patch should be applied as is. We can add further improvements
>> later.
>>
>> Acked-by: Cian Ferriter <cian.ferri...@intel.com>
>>
>> More details about what I tested:
>> I switched to GCC 5 on my system and can see the compile issue below before
>> applying the patch.
>> After applying the patch, the build is successful.
>> Before patch
>> Configure message:
>> checking whether gcc accepts -mavx512f... yes
>>
>> Build error:
>> make[3]: Entering directory '/root/cian/ovs/datapath'
>> lib/dpif-netdev-lookup-avx512-gather.c:90:1: error:
>> attribute(target("avx512vpopcntdq")) is unknown
>>
>> After patch
>> Configure message:
>> checking whether gcc accepts -mavx512f -mavx512vpopcntdq... no
>>
>> On GCC 4.8 before the patch is applied, the build is actually successful,
>> since the
>> "avx512f" only check fails, so the AVX512 build is disabled.
>> On GCC 4.8 after the patch is applied, the build still works as expected.
>> Before patch
>> Configure message:
>> checking whether gcc -std=gnu99 accepts -mavx512f... no
>>
>> After patch
>> Configure message:
>> checking whether gcc -std=gnu99 accepts -mavx512f -mavx512vpopcntdq... no
>>
>> I also checked that all was working as expected with the compile time AVX512
>> flags:
>> --enable-dpif-default-avx512 --enable-autovalidator --enable-mfex-default-
>> autovalidator
>>
>> I can confirm that these compile time flags don't cause issues.
>>
>> Finally, I tested and confirmed that OVS was still building and running with
>> AVX512 support when built with GCC 9.
>
> Thanks for the patch William and thanks for testing Cian.
>
> Just looking and it seems this should be backported as far as 2.16 if I'm
> correct? At least from code inspection I see reference to vpopcntdq mainly
> around dpif (If memory serves me correctly it was 2.16 when AVX512 was
> introduced to dpif).
>
> Thoughts?
Yes, the backport is needed. I believe we had this issue reported
for 2.16 on IRC in September, but we didn't have enough details,
and no real actions followed:
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387675.html
>
> Thanks
> Ian
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
