[dpdk-dev] [PATCH v2 07/12] acl: add infrastructure to support AVX512 classify

[Bruce Richardson]

On Wed, Sep 16, 2020 at 10:36:32AM +0100, Medvedkin, Vladimir wrote:
> Hi Bruce,
> 
> On 16/09/2020 10:11, Bruce Richardson wrote:
> > On Tue, Sep 15, 2020 at 05:50:20PM +0100, Konstantin Ananyev wrote:
> > > Add necessary changes to support new AVX512 specific ACL classify
> > > algorithm:
> > >   - changes in meson.build to check that build tools
> > >     (compiler, assembler, etc.) do properly support AVX512.
> > >   - run-time checks to make sure target platform does support AVX512.
> > >   - dummy rte_acl_classify_avx512() for targets where AVX512
> > >     implementation couldn't be properly supported.
> > > 
> > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> > > ---
> > 
> > This all looks correct, though I wonder do you really need to check all
> > those AVX512 flags in each case? Since "F" is always present in any AVX512
> > implementation perhaps it can be checked, though if the other three always
> > need to be checked I can understand if you want to keep it there for
> > completeness. [Are all the other 3 used in your code?]
> > 
> 
> As for me it is good to check all the flags supported by compiler. Some old
> (but still supported by dpdk) gcc can't compile the code in some
> circumstances. For example:
> 
> gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.12)   <-- pretty old
> but still supported, right?
> 
> gcc -march=native -dM -E - < /dev/null | grep "AVX512"
> #define __AVX512F__ 1
> #define __AVX512BW__ 1
> #define __AVX512CD__ 1
> #define __AVX512DQ__ 1
> 
> Does not support __AVX512VL__
> 
Interesting, seems like checking them all to be sure is the right approach
so.
My ack stands so, and ignore the comment.