From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 41C135A35 for ; Fri, 20 Mar 2015 13:41:52 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 20 Mar 2015 05:41:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,436,1422950400"; d="scan'208";a="668118423" Received: from unknown (HELO [10.217.248.136]) ([10.217.248.136]) by orsmga001.jf.intel.com with ESMTP; 20 Mar 2015 05:41:50 -0700 Message-ID: <550C1563.1040807@intel.com> Date: Fri, 20 Mar 2015 13:41:07 +0100 From: Pawel Wodkowski User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: dev@dpdk.org References: In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] hash: fix breaking strict-aliasing rules X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Mar 2015 12:41:53 -0000 On 2015-03-18 17:51, Yerden Zhumabekov wrote: > Fix rte_hash_crc() function. Casting uint64_t pointer to uin32_t > may trigger a compiler warning about breaking strict-aliasing rules. > To avoid that, introduce a lookup table which is used to mask out > a remainder of data. > > See issue #1, http://dpdk.org/ml/archives/dev/2015-March/015174.html > > Signed-off-by: Yerden Zhumabekov > --- > lib/librte_hash/rte_hash_crc.h | 31 +++++++++++++++---------------- > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h > index 3dcd362..e81920f 100644 > --- a/lib/librte_hash/rte_hash_crc.h > +++ b/lib/librte_hash/rte_hash_crc.h > @@ -323,6 +323,16 @@ static const uint32_t crc32c_tables[8][256] = {{ > 0xE54C35A1, 0xAC704886, 0x7734CFEF, 0x3E08B2C8, 0xC451B7CC, 0x8D6DCAEB, 0x56294D82, 0x1F1530A5 > }}; > > +static const uint64_t odd_8byte_mask[] = { > + 0x00FFFFFFFFFFFFFF, > + 0x0000FFFFFFFFFFFF, > + 0x000000FFFFFFFFFF, > + 0x00000000FFFFFFFF, > + 0x0000000000FFFFFF, > + 0x000000000000FFFF, > + 0x00000000000000FF, > +}; > + > #define CRC32_UPD(crc, n) \ > (crc32c_tables[(n)][(crc) & 0xFF] ^ \ > crc32c_tables[(n)-1][((crc) >> 8) & 0xFF]) > @@ -535,38 +545,27 @@ static inline uint32_t > rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val) > { > unsigned i; > - uint64_t temp = 0; > + uint64_t temp; > const uint64_t *p64 = (const uint64_t *)data; > > for (i = 0; i < data_len / 8; i++) { > init_val = rte_hash_crc_8byte(*p64++, init_val); > } > > - switch (7 - (data_len & 0x07)) { > + i = 7 - (data_len & 0x07); Great idea with lookup table! Only one question here: why keeping this magic at all now? If you sort odd_8byte_mask opposite direction you can do something like that data_len &= 0x07; switch (data_len & 0x07) { case 1: case 2: case 3: case 4: temp = odd_8byte_mask[data_len] & *p64; init_val = rte_hash_crc_4byte(temp, init_val); case 5: case 6: case 7: temp = odd_8byte_mask[data_len] & *p64; init_val = rte_hash_crc_8byte(temp, init_val); break; default: break; } Or data_len &= 0x07; if (data_len > 0) { temp = odd_8byte_mask[data_len] & *p64; if (data_len <= 4) init_val = rte_hash_crc_4byte(temp, init_val); else init_val = rte_hash_crc_8byte(temp, init_val); } Is there something obvious what I am not seeing here? Pawel > + switch (i) { > case 0: > - temp |= (uint64_t) *((const uint8_t *)p64 + 6) << 48; > - /* Fallthrough */ > case 1: > - temp |= (uint64_t) *((const uint8_t *)p64 + 5) << 40; > - /* Fallthrough */ > case 2: > - temp |= (uint64_t) *((const uint8_t *)p64 + 4) << 32; > - temp |= *((const uint32_t *)p64); > + temp = odd_8byte_mask[i] & *p64; > init_val = rte_hash_crc_8byte(temp, init_val); > break; > case 3: > - init_val = rte_hash_crc_4byte(*(const uint32_t *)p64, init_val); > - break; > case 4: > - temp |= *((const uint8_t *)p64 + 2) << 16; > - /* Fallthrough */ > case 5: > - temp |= *((const uint8_t *)p64 + 1) << 8; > - /* Fallthrough */ > case 6: > - temp |= *((const uint8_t *)p64); > + temp = odd_8byte_mask[i] & *p64; > init_val = rte_hash_crc_4byte(temp, init_val); > - /* Fallthrough */ > default: > break; > } > -- Pawel