| Message ID | 20210729165434.2773795-1-harry.van.haaren@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpcls: fix build on compilers without AVX512-VPOPCNT | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
> -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Thursday, July 29, 2021 5:55 PM > To: ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry > <harry.van.haaren@intel.com> > Subject: [PATCH] dpcls: fix build on compilers without AVX512-VPOPCNT > > This commit adds extra checks around the AVX-512 vpopcnt instruction > enabling, ensuring that in the function where the ISA is enabled the > compiler has also indicated its support for the ISA. This is achieved > by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if > it is capable of handling the vpopcnt instruction. > > If the compiler is not capable of handling vpopcnt, we fall back to > the emulated vpopcnt implementation. > > Reported-by: Ian Stokes <ian.stokes@intel.com> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > Based on a very old system with GCC 7, an issue was identified > where the compiler doesn't support the vpopcnt ISA, and resulted > in compilation failures. Ping on this patch, would be good to get integrated on 2.16 and master to ensure Gcc7 builds correctly.
> > -----Original Message----- > > From: Van Haaren, Harry <harry.van.haaren@intel.com> > > Sent: Thursday, July 29, 2021 5:55 PM > > To: ovs-dev@openvswitch.org > > Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry > > <harry.van.haaren@intel.com> > > Subject: [PATCH] dpcls: fix build on compilers without AVX512-VPOPCNT > > > > This commit adds extra checks around the AVX-512 vpopcnt instruction > > enabling, ensuring that in the function where the ISA is enabled the > > compiler has also indicated its support for the ISA. This is achieved > > by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if > > it is capable of handling the vpopcnt instruction. > > > > If the compiler is not capable of handling vpopcnt, we fall back to > > the emulated vpopcnt implementation. > > > > Reported-by: Ian Stokes <ian.stokes@intel.com> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > --- > > > > Based on a very old system with GCC 7, an issue was identified > > where the compiler doesn't support the vpopcnt ISA, and resulted > > in compilation failures. > > Ping on this patch, would be good to get integrated on 2.16 and master to > ensure Gcc7 builds correctly. HI Harry, Just testing this now. Regards Ian
On 8/10/21 12:11 PM, Stokes, Ian wrote: >>> -----Original Message----- >>> From: Van Haaren, Harry <harry.van.haaren@intel.com> >>> Sent: Thursday, July 29, 2021 5:55 PM >>> To: ovs-dev@openvswitch.org >>> Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry >>> <harry.van.haaren@intel.com> >>> Subject: [PATCH] dpcls: fix build on compilers without AVX512-VPOPCNT >>> >>> This commit adds extra checks around the AVX-512 vpopcnt instruction >>> enabling, ensuring that in the function where the ISA is enabled the >>> compiler has also indicated its support for the ISA. This is achieved >>> by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if >>> it is capable of handling the vpopcnt instruction. >>> >>> If the compiler is not capable of handling vpopcnt, we fall back to >>> the emulated vpopcnt implementation. >>> >>> Reported-by: Ian Stokes <ian.stokes@intel.com> >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >>> >>> --- >>> >>> Based on a very old system with GCC 7, an issue was identified >>> where the compiler doesn't support the vpopcnt ISA, and resulted >>> in compilation failures. >> >> Ping on this patch, would be good to get integrated on 2.16 and master to >> ensure Gcc7 builds correctly. > > HI Harry, > > Just testing this now. > > Regards > Ian FWIW, I tried to test this yesterday, but I realized that you need gcc 7.0.0 for this, because AVX512VPOPCNTDQ is supported starting 7.0.1. That's a very weird system you have. :) Unfortunately, I realized this too late when I already built 7.5.0 from sources and I didn't want to waste another 40 mins building 7.0.0. So, I carved out support for AVX512VPOPCNTDQ from gcc 7.5.0. In this configuration it failed to build OVS with the following error: lib/dpif-netdev-lookup-avx512-gather.c: In function ‘_mm512_popcnt_epi64_wrapper’: lib/dpif-netdev-lookup-avx512-gather.c:62:12: error: implicit declaration of function ‘_mm512_popcnt_epi64’; did you mean ‘_mm512_lzcnt_epi64’? [-Werror=implicit-function-declaration] return _mm512_popcnt_epi64(v_in); ^~~~~~~~~~~~~~~~~~~ _mm512_lzcnt_epi64 With the patch applied, this modified gcc was able to build OVS successfully. Best regards, Ilya Maximets.
> On 8/10/21 12:11 PM, Stokes, Ian wrote: > >>> -----Original Message----- > >>> From: Van Haaren, Harry <harry.van.haaren@intel.com> > >>> Sent: Thursday, July 29, 2021 5:55 PM > >>> To: ovs-dev@openvswitch.org > >>> Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry > >>> <harry.van.haaren@intel.com> > >>> Subject: [PATCH] dpcls: fix build on compilers without AVX512-VPOPCNT > >>> > >>> This commit adds extra checks around the AVX-512 vpopcnt instruction > >>> enabling, ensuring that in the function where the ISA is enabled the > >>> compiler has also indicated its support for the ISA. This is achieved > >>> by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if > >>> it is capable of handling the vpopcnt instruction. > >>> > >>> If the compiler is not capable of handling vpopcnt, we fall back to > >>> the emulated vpopcnt implementation. > >>> > >>> Reported-by: Ian Stokes <ian.stokes@intel.com> > >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > >>> > >>> --- > >>> > >>> Based on a very old system with GCC 7, an issue was identified > >>> where the compiler doesn't support the vpopcnt ISA, and resulted > >>> in compilation failures. > >> > >> Ping on this patch, would be good to get integrated on 2.16 and master to > >> ensure Gcc7 builds correctly. > > > > HI Harry, > > > > Just testing this now. > > > > Regards > > Ian > > FWIW, > I tried to test this yesterday, but I realized that you need gcc 7.0.0 > for this, because AVX512VPOPCNTDQ is supported starting 7.0.1. > That's a very weird system you have. :) > > Unfortunately, I realized this too late when I already built 7.5.0 from > sources and I didn't want to waste another 40 mins building 7.0.0. > So, I carved out support for AVX512VPOPCNTDQ from gcc 7.5.0. > In this configuration it failed to build OVS with the following error: > > lib/dpif-netdev-lookup-avx512-gather.c: In function > ‘_mm512_popcnt_epi64_wrapper’: > lib/dpif-netdev-lookup-avx512-gather.c:62:12: error: implicit declaration of > function ‘_mm512_popcnt_epi64’; did you mean ‘_mm512_lzcnt_epi64’? [- > Werror=implicit-function-declaration] > return _mm512_popcnt_epi64(v_in); > ^~~~~~~~~~~~~~~~~~~ > _mm512_lzcnt_epi64 > > With the patch applied, this modified gcc was able to build OVS > successfully. Thanks for checking Ilya. I've an older system in our lab that I was able to check and reproduce the issue and confirm it resolves the issue. I'll add a fixes tag and apply to master and branch 2.16. Thanks Ian > > Best regards, Ilya Maximets.
> > This commit adds extra checks around the AVX-512 vpopcnt instruction > > enabling, ensuring that in the function where the ISA is enabled the > > compiler has also indicated its support for the ISA. This is achieved > > by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if > > it is capable of handling the vpopcnt instruction. > > > > If the compiler is not capable of handling vpopcnt, we fall back to > > the emulated vpopcnt implementation. > > > > Reported-by: Ian Stokes <ian.stokes@intel.com> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > --- > > > > Based on a very old system with GCC 7, an issue was identified > > where the compiler doesn't support the vpopcnt ISA, and resulted > > in compilation failures. > > Ping on this patch, would be good to get integrated on 2.16 and master to > ensure Gcc7 builds correctly. Thanks, tested OK, applied to mast and branch 2.16 Regards Ian
diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c index ced846aa77..072831e96a 100644 --- a/lib/dpif-netdev-lookup-avx512-gather.c +++ b/lib/dpif-netdev-lookup-avx512-gather.c @@ -53,15 +53,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_lookup_avx512_gather); - -/* Wrapper function required to enable ISA. */ -static inline __m512i -__attribute__((__target__("avx512vpopcntdq"))) -_mm512_popcnt_epi64_wrapper(__m512i v_in) -{ - return _mm512_popcnt_epi64(v_in); -} - static inline __m512i _mm512_popcnt_epi64_manual(__m512i v_in) { @@ -85,6 +76,23 @@ _mm512_popcnt_epi64_manual(__m512i v_in) return _mm512_sad_epu8(v_u8_pop, _mm512_setzero_si512()); } +/* Wrapper function required to enable ISA. First enable the ISA via the + * attribute target for this function, then check if the compiler actually + * #defines the ISA itself. If the ISA is not #define-ed by the compiler it + * indicates the compiler is too old or is not capable of compiling the + * requested ISA level, so fallback to the integer manual implementation. + */ +static inline __m512i +__attribute__((__target__("avx512vpopcntdq"))) +_mm512_popcnt_epi64_wrapper(__m512i v_in) +{ +#ifdef __AVX512VPOPCNTDQ__ + return _mm512_popcnt_epi64(v_in); +#else + return _mm512_popcnt_epi64_manual(v_in); +#endif +} + static inline uint64_t netdev_rule_matches_key(const struct dpcls_rule *rule, const uint32_t mf_bits_total,
This commit adds extra checks around the AVX-512 vpopcnt instruction enabling, ensuring that in the function where the ISA is enabled the compiler has also indicated its support for the ISA. This is achieved by checking the __AVX512VPOPCNTDQ__ define, which the compiler sets if it is capable of handling the vpopcnt instruction. If the compiler is not capable of handling vpopcnt, we fall back to the emulated vpopcnt implementation. Reported-by: Ian Stokes <ian.stokes@intel.com> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- Based on a very old system with GCC 7, an issue was identified where the compiler doesn't support the vpopcnt ISA, and resulted in compilation failures. --- lib/dpif-netdev-lookup-avx512-gather.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)