Re: [dpdk-dev] [PATCH 1/2] net/i40e: fix generic build on FreeBSD

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/18/2021 5:42 PM, Bruce Richardson wrote:
> On Wed, Aug 18, 2021 at 05:38:15PM +0100, Bruce Richardson wrote:
>> The common header file for vectorization is included in multiple files,
>> and so must use macros for the current compilation unit, rather than the
>> compiler-capability flag set for the whole driver. With the current,
>> incorrect, macro, the AVX512 or AVX2 flags may be set when compiling up
>> SSE code, leading to compilation errors. Changing from "CC_AVX*_SUPPORT"
>> to the compiler-defined "__AVX*__" macros fixes this issue.
>>
>> Bugzilla ID: 788
>> Fixes: 0604b1f2208f ("net/i40e: fix crash in AVX512")
>> Cc: wenzhuo.lu@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
>> index f52ed98d62..65715ed1ce 100644
>> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
>> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
>> @@ -268,7 +268,7 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>>  #endif
>>  }
>>  
>> -#ifdef CC_AVX2_SUPPORT
>> +#ifdef __AVX2__
>>  static __rte_always_inline void
>>  i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool avx512)
>>  {
> 
> On a higher-level, I'd suggest we look to remove the use of these macros
> and AVX code in general from this header file (and ice driver equivalent).
> IIRC this file was originally meant to contain only the "common" code i.e.
> the scalar code, to be shared among vector implementations. Having AVX code
> in this file can lead to these sorts of bugs and just makes the file no
> longer truely common. The code in question here, should probably go in a
> "common_avx" header, which means that we can remove the AVX2 conditions
> from it etc.
> 

Indeed I come to this thread to make exact same comment, I think better to move
avx specific code into a "common_avx" header.