| Message ID | 1449486553-16699-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Accepted |
| Commit | fc28b868861fd11dbfd1a3a4cb5ee0338f88eaf9 |
| Headers | show |
shift still is not fixed:
odp_name_table.c:186:4: error: right shift count >= width of type [-Werror]
if ((hash_tbl_entry >> 48) == 0x7FFF)
^
odp_name_table.c:188:4: error: right shift count >= width of type [-Werror]
else if ((hash_tbl_entry >> 48) == 0)
On 12/07/2015 14:09, Bill Fischofer wrote:
> Change the internal hash_name_and_kind() function to eliminate the use
> of architecture-specific hash instructions. This eliminates build issues
> surrounding ARM variants. Future optimizations will use the arch directory
> consistent with other ODP modules.
>
> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
> ---
> platform/linux-generic/odp_name_table.c | 184 +-------------------------------
> 1 file changed, 2 insertions(+), 182 deletions(-)
>
> diff --git a/platform/linux-generic/odp_name_table.c b/platform/linux-generic/odp_name_table.c
> index 10a760e..b4a9081 100644
> --- a/platform/linux-generic/odp_name_table.c
> +++ b/platform/linux-generic/odp_name_table.c
> @@ -48,140 +48,6 @@
>
> #define SECONDARY_HASH_HISTO_PRINT 1
>
> - /* #define USE_AES */
> -
> -#if defined __x86_64__ || defined __i386__
> -
> -#ifdef USE_AES
> -
> -typedef long long int v2di __attribute__((vector_size(16)));
> -
> -static const v2di HASH_CONST1 = { 0x123456, 0xFEBCDA383 };
> -static const v2di HASH_CONST2 = { 0x493BA3F689, 0x102F5D73A8C };
> -
> -#define PLATFORM_HASH_STATE v2di
> -
> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \
> - ({ \
> - hash_state = HASH_CONST1; \
> - hash_state[0] ^= name_len; \
> - })
> -
> -#define PLATFORM_HASH32(hash_state, name_word) \
> - ({ \
> - v2di data; \
> - \
> - data[0] = name_word; \
> - data[1] = name_word << 1; \
> - hash_state = __builtin_ia32_aesenc128(hash_state, data); \
> - })
> -
> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \
> - ({ \
> - uint64_t result; \
> - v2di data; \
> - \
> - data[0] = name_kind; \
> - data[1] = name_kind << 7; \
> - hash_state = __builtin_ia32_aesenc128(hash_state, data); \
> - hash_state = __builtin_ia32_aesenc128(hash_state, \
> - HASH_CONST2); \
> - hash_state = __builtin_ia32_aesenc128(hash_state, \
> - HASH_CONST1); \
> - result = (uint64_t)hash_state[0] ^ hash_state[1]; \
> - result = result ^ result >> 32; \
> - (uint32_t)result; \
> - })
> -
> -#else
> -
> -#define PLATFORM_HASH_STATE uint64_t
> -
> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \
> - ({ \
> - hash_state = (uint64_t)name_len; \
> - hash_state |= hash_state << 8; \
> - hash_state |= hash_state << 16; \
> - hash_state |= hash_state << 32; \
> - })
> -
> -#define PLATFORM_HASH32(hash_state, name_word) \
> - ({ \
> - uint64_t temp; \
> - \
> - temp = ((uint64_t)name_word) * 0xFEFDFCF5; \
> - hash_state = hash_state * 0xFF; \
> - hash_state ^= temp ^ (uint64_t)name_word; \
> - })
> -
> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \
> - ({ \
> - hash_state ^= (((uint32_t)kind) << 13); \
> - hash_state = hash_state * 0xFEFDFCF5; \
> - hash_state = hash_state ^ hash_state >> 32; \
> - hash_state = hash_state % 0xFEEDDCCBBAA1; \
> - hash_state = hash_state ^ hash_state >> 32; \
> - (uint32_t)hash_state; \
> - })
> -
> -#endif
> -
> -#elif defined(__tile_gx__)
> -
> -#define PLATFORM_HASH_STATE uint32_t
> -
> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \
> - ({ \
> - hash_state = 0xFEFDFCF5; \
> - hash_state = __insn_crc_32_32(hash_state, name_len); \
> - })
> -
> -#define PLATFORM_HASH32(hash_state, name_word) \
> - ({ \
> - hash_state = __insn_crc_32_32(hash_state, name_word); \
> - })
> -
> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \
> - ({ \
> - hash_state = __insn_crc_32_32(hash_state, kind); \
> - hash_state = __insn_crc_32_32(hash_state, 0xFFFFFFFF); \
> - hash_state = __insn_crc_32_32(hash_state, 0xFEFDFCF5); \
> - (uint32_t)hash_state; \
> - })
> -
> -#elif defined(__arm__) || defined(__aarch64__)
> -
> -#define PLATFORM_HASH_STATE uint32_t
> -
> -static inline uint32_t __crc32w(uint32_t crc, uint32_t value)
> -{
> - __asm__("crc32w %w[c], %w[c], %w[v]":[c]"+r"(crc):[v]"r"(value));
> - return crc;
> -}
> -
> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \
> - ({ \
> - hash_state = 0xFEFDFCF5; \
> - hash_state = __crc32w(hash_state, name_len); \
> - })
> -
> -#define PLATFORM_HASH32(hash_state, name_word) \
> - ({ \
> - __crc32w(hash_state, name_word); \
> - })
> -
> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \
> - ({ \
> - hash_state = __crc32w(hash_state, kind); \
> - hash_state = __crc32w(hash_state, 0xFFFFFFFF); \
> - hash_state = __crc32w(hash_state, 0xFEFDFCF5); \
> - (uint32_t)hash_state; \
> - })
> -
> -#else
> -#error "Need to define PLATFORM_DEPENDENT_HASH32 macro"
> -#endif
> -
> typedef struct name_tbl_entry_s name_tbl_entry_t;
>
> /* It is important for most platforms that the following struct fit within
> @@ -227,7 +93,7 @@ typedef struct {
> * NOT zero then this values points to a (linked list of) name_table_entry_t
> * records AND the bottom 6 bits are the length of this list.
> */
> -typedef uint64_t hash_tbl_entry_t;
> +typedef uintptr_t hash_tbl_entry_t;
>
> typedef struct {
> hash_tbl_entry_t hash_entries[SECONDARY_HASH_TBL_SIZE];
> @@ -275,53 +141,7 @@ static void aligned_free(void *mem_ptr)
>
> static uint32_t hash_name_and_kind(const char *name, uint8_t name_kind)
> {
> - PLATFORM_HASH_STATE hash_state;
> - uint32_t name_len, name_word, hash_value;
> - uint32_t bytes[4];
> -
> - name_len = strlen(name);
> - PLATFORM_HASH32_INIT(hash_state, name_len);
> -
> - while (4 <= name_len) {
> - /* Get the next four characters. Note that endianness doesn't
> - * matter here! Also note that this assumes that there is
> - * either no alignment loading restrictions OR that name is
> - * 32-bit aligned. Shouldn't be too hard to add code to deal
> - * with the case when this assumption is false.
> - */
> - /* name_word = *((uint32_t *)name); */
> - bytes[0] = name[0];
> - bytes[1] = name[1];
> - bytes[2] = name[2];
> - bytes[3] = name[3];
> - name_word = (bytes[3] << 24) | (bytes[2] << 16) |
> - (bytes[1] << 8) | bytes[0];
> - PLATFORM_HASH32(hash_state, name_word);
> -
> - name_len -= 4;
> - name += 4;
> - }
> -
> - if (name_len != 0) {
> - name_word = 0;
> -
> - if (2 <= name_len) {
> - /* name_word = (uint32_t)*((uint16_t *)name); */
> - bytes[0] = name[0];
> - bytes[1] = name[1];
> - name_word |= (bytes[1] << 8) | bytes[0];
> - name_len -= 2;
> - name += 2;
> - }
> -
> - if (name_len == 1)
> - name_word |= ((uint32_t)*name) << 16;
> -
> - PLATFORM_HASH32(hash_state, name_word);
> - }
> -
> - hash_value = PLATFORM_HASH32_FINISH(hash_state, name_kind);
> - return hash_value;
> + return odp_hash_crc32c(name, strlen(name), name_kind);
> }
>
> static uint32_t linked_list_len(name_tbl_entry_t *name_tbl_entry)
v1 should not have that problem. What error do you get when trying that? check-odp with M32_ON_64 works fine for me. On Mon, Dec 7, 2015 at 9:01 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > shift still is not fixed: > > odp_name_table.c:186:4: error: right shift count >= width of type [-Werror] > if ((hash_tbl_entry >> 48) == 0x7FFF) > ^ > odp_name_table.c:188:4: error: right shift count >= width of type [-Werror] > else if ((hash_tbl_entry >> 48) == 0) > > > > On 12/07/2015 14:09, Bill Fischofer wrote: > >> Change the internal hash_name_and_kind() function to eliminate the use >> of architecture-specific hash instructions. This eliminates build issues >> surrounding ARM variants. Future optimizations will use the arch directory >> consistent with other ODP modules. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/odp_name_table.c | 184 >> +------------------------------- >> 1 file changed, 2 insertions(+), 182 deletions(-) >> >> diff --git a/platform/linux-generic/odp_name_table.c >> b/platform/linux-generic/odp_name_table.c >> index 10a760e..b4a9081 100644 >> --- a/platform/linux-generic/odp_name_table.c >> +++ b/platform/linux-generic/odp_name_table.c >> @@ -48,140 +48,6 @@ >> #define SECONDARY_HASH_HISTO_PRINT 1 >> - /* #define USE_AES */ >> - >> -#if defined __x86_64__ || defined __i386__ >> - >> -#ifdef USE_AES >> - >> -typedef long long int v2di __attribute__((vector_size(16))); >> - >> -static const v2di HASH_CONST1 = { 0x123456, 0xFEBCDA383 }; >> -static const v2di HASH_CONST2 = { 0x493BA3F689, 0x102F5D73A8C }; >> - >> -#define PLATFORM_HASH_STATE v2di >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = HASH_CONST1; \ >> - hash_state[0] ^= name_len; \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - v2di data; \ >> - \ >> - data[0] = name_word; \ >> - data[1] = name_word << 1; \ >> - hash_state = __builtin_ia32_aesenc128(hash_state, data); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - uint64_t result; \ >> - v2di data; \ >> - \ >> - data[0] = name_kind; \ >> - data[1] = name_kind << 7; \ >> - hash_state = __builtin_ia32_aesenc128(hash_state, data); \ >> - hash_state = __builtin_ia32_aesenc128(hash_state, \ >> - HASH_CONST2); \ >> - hash_state = __builtin_ia32_aesenc128(hash_state, \ >> - HASH_CONST1); \ >> - result = (uint64_t)hash_state[0] ^ hash_state[1]; \ >> - result = result ^ result >> 32; \ >> - (uint32_t)result; \ >> - }) >> - >> -#else >> - >> -#define PLATFORM_HASH_STATE uint64_t >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = (uint64_t)name_len; \ >> - hash_state |= hash_state << 8; \ >> - hash_state |= hash_state << 16; \ >> - hash_state |= hash_state << 32; \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - uint64_t temp; \ >> - \ >> - temp = ((uint64_t)name_word) * 0xFEFDFCF5; \ >> - hash_state = hash_state * 0xFF; \ >> - hash_state ^= temp ^ (uint64_t)name_word; \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state ^= (((uint32_t)kind) << 13); \ >> - hash_state = hash_state * 0xFEFDFCF5; \ >> - hash_state = hash_state ^ hash_state >> 32; \ >> - hash_state = hash_state % 0xFEEDDCCBBAA1; \ >> - hash_state = hash_state ^ hash_state >> 32; \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#endif >> - >> -#elif defined(__tile_gx__) >> - >> -#define PLATFORM_HASH_STATE uint32_t >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = 0xFEFDFCF5; \ >> - hash_state = __insn_crc_32_32(hash_state, name_len); \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - hash_state = __insn_crc_32_32(hash_state, name_word); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state = __insn_crc_32_32(hash_state, kind); \ >> - hash_state = __insn_crc_32_32(hash_state, 0xFFFFFFFF); \ >> - hash_state = __insn_crc_32_32(hash_state, 0xFEFDFCF5); \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#elif defined(__arm__) || defined(__aarch64__) >> - >> -#define PLATFORM_HASH_STATE uint32_t >> - >> -static inline uint32_t __crc32w(uint32_t crc, uint32_t value) >> -{ >> - __asm__("crc32w %w[c], %w[c], %w[v]":[c]"+r"(crc):[v]"r"(value)); >> - return crc; >> -} >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = 0xFEFDFCF5; \ >> - hash_state = __crc32w(hash_state, name_len); \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - __crc32w(hash_state, name_word); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state = __crc32w(hash_state, kind); \ >> - hash_state = __crc32w(hash_state, 0xFFFFFFFF); \ >> - hash_state = __crc32w(hash_state, 0xFEFDFCF5); \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#else >> -#error "Need to define PLATFORM_DEPENDENT_HASH32 macro" >> -#endif >> - >> typedef struct name_tbl_entry_s name_tbl_entry_t; >> /* It is important for most platforms that the following struct fit >> within >> @@ -227,7 +93,7 @@ typedef struct { >> * NOT zero then this values points to a (linked list of) >> name_table_entry_t >> * records AND the bottom 6 bits are the length of this list. >> */ >> -typedef uint64_t hash_tbl_entry_t; >> +typedef uintptr_t hash_tbl_entry_t; >> typedef struct { >> hash_tbl_entry_t hash_entries[SECONDARY_HASH_TBL_SIZE]; >> @@ -275,53 +141,7 @@ static void aligned_free(void *mem_ptr) >> static uint32_t hash_name_and_kind(const char *name, uint8_t >> name_kind) >> { >> - PLATFORM_HASH_STATE hash_state; >> - uint32_t name_len, name_word, hash_value; >> - uint32_t bytes[4]; >> - >> - name_len = strlen(name); >> - PLATFORM_HASH32_INIT(hash_state, name_len); >> - >> - while (4 <= name_len) { >> - /* Get the next four characters. Note that endianness >> doesn't >> - * matter here! Also note that this assumes that there is >> - * either no alignment loading restrictions OR that name >> is >> - * 32-bit aligned. Shouldn't be too hard to add code to >> deal >> - * with the case when this assumption is false. >> - */ >> - /* name_word = *((uint32_t *)name); */ >> - bytes[0] = name[0]; >> - bytes[1] = name[1]; >> - bytes[2] = name[2]; >> - bytes[3] = name[3]; >> - name_word = (bytes[3] << 24) | (bytes[2] << 16) | >> - (bytes[1] << 8) | bytes[0]; >> - PLATFORM_HASH32(hash_state, name_word); >> - >> - name_len -= 4; >> - name += 4; >> - } >> - >> - if (name_len != 0) { >> - name_word = 0; >> - >> - if (2 <= name_len) { >> - /* name_word = (uint32_t)*((uint16_t *)name); */ >> - bytes[0] = name[0]; >> - bytes[1] = name[1]; >> - name_word |= (bytes[1] << 8) | bytes[0]; >> - name_len -= 2; >> - name += 2; >> - } >> - >> - if (name_len == 1) >> - name_word |= ((uint32_t)*name) << 16; >> - >> - PLATFORM_HASH32(hash_state, name_word); >> - } >> - >> - hash_value = PLATFORM_HASH32_FINISH(hash_state, name_kind); >> - return hash_value; >> + return odp_hash_crc32c(name, strlen(name), name_kind); >> } >> static uint32_t linked_list_len(name_tbl_entry_t *name_tbl_entry) >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/07/2015 20:14, Bill Fischofer wrote: > v1 should not have that problem. What error do you get when trying > that? check-odp with M32_ON_64 works fine for me. export GIT_ODP=/opt/Linaro/odp2.git export CLEANUP=1 export GIT_BRANCH=`(cd $GIT_ODP; git symbolic-ref HEAD | cut -d / -f 3)` ./build-all.sh > > On Mon, Dec 7, 2015 at 9:01 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > shift still is not fixed: > > odp_name_table.c:186:4: error: right shift count >= width of type > [-Werror] > if ((hash_tbl_entry >> 48) == 0x7FFF) > ^ > odp_name_table.c:188:4: error: right shift count >= width of type > [-Werror] > else if ((hash_tbl_entry >> 48) == 0) > > > > On 12/07/2015 14:09, Bill Fischofer wrote: > > Change the internal hash_name_and_kind() function to eliminate > the use > of architecture-specific hash instructions. This eliminates > build issues > surrounding ARM variants. Future optimizations will use the > arch directory > consistent with other ODP modules. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > --- > platform/linux-generic/odp_name_table.c | 184 > +------------------------------- > 1 file changed, 2 insertions(+), 182 deletions(-) > > diff --git a/platform/linux-generic/odp_name_table.c > b/platform/linux-generic/odp_name_table.c > index 10a760e..b4a9081 100644 > --- a/platform/linux-generic/odp_name_table.c > +++ b/platform/linux-generic/odp_name_table.c > @@ -48,140 +48,6 @@ > #define SECONDARY_HASH_HISTO_PRINT 1 > - /* #define USE_AES */ > - > -#if defined __x86_64__ || defined __i386__ > - > -#ifdef USE_AES > - > -typedef long long int v2di __attribute__((vector_size(16))); > - > -static const v2di HASH_CONST1 = { 0x123456, 0xFEBCDA383 }; > -static const v2di HASH_CONST2 = { 0x493BA3F689, 0x102F5D73A8C }; > - > -#define PLATFORM_HASH_STATE v2di > - > -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ > - ({ \ > - hash_state = HASH_CONST1; \ > - hash_state[0] ^= name_len; \ > - }) > - > -#define PLATFORM_HASH32(hash_state, name_word) \ > - ({ \ > - v2di data; \ > - \ > - data[0] = name_word; \ > - data[1] = name_word << 1; > \ > - hash_state = > __builtin_ia32_aesenc128(hash_state, data); \ > - }) > - > -#define PLATFORM_HASH32_FINISH(hash_state, kind) > \ > - ({ \ > - uint64_t result; \ > - v2di data; \ > - \ > - data[0] = name_kind; \ > - data[1] = name_kind << 7; > \ > - hash_state = > __builtin_ia32_aesenc128(hash_state, data); \ > - hash_state = > __builtin_ia32_aesenc128(hash_state, \ > - HASH_CONST2); \ > - hash_state = > __builtin_ia32_aesenc128(hash_state, \ > - HASH_CONST1); \ > - result = (uint64_t)hash_state[0] ^ > hash_state[1]; \ > - result = result ^ result >> 32; > \ > - (uint32_t)result; \ > - }) > - > -#else > - > -#define PLATFORM_HASH_STATE uint64_t > - > -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ > - ({ \ > - hash_state = (uint64_t)name_len; \ > - hash_state |= hash_state << 8; \ > - hash_state |= hash_state << 16; \ > - hash_state |= hash_state << 32; \ > - }) > - > -#define PLATFORM_HASH32(hash_state, name_word) \ > - ({ \ > - uint64_t temp; \ > - \ > - temp = ((uint64_t)name_word) * > 0xFEFDFCF5; \ > - hash_state = hash_state * 0xFF; > \ > - hash_state ^= temp ^ (uint64_t)name_word; > \ > - }) > - > -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ > - ({ \ > - hash_state ^= (((uint32_t)kind) << 13); \ > - hash_state = hash_state * 0xFEFDFCF5; \ > - hash_state = hash_state ^ hash_state >> 32; \ > - hash_state = hash_state % 0xFEEDDCCBBAA1; \ > - hash_state = hash_state ^ hash_state >> 32; \ > - (uint32_t)hash_state; \ > - }) > - > -#endif > - > -#elif defined(__tile_gx__) > - > -#define PLATFORM_HASH_STATE uint32_t > - > -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ > - ({ \ > - hash_state = 0xFEFDFCF5; \ > - hash_state = __insn_crc_32_32(hash_state, > name_len); \ > - }) > - > -#define PLATFORM_HASH32(hash_state, name_word) \ > - ({ \ > - hash_state = __insn_crc_32_32(hash_state, > name_word); \ > - }) > - > -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ > - ({ \ > - hash_state = __insn_crc_32_32(hash_state, > kind); \ > - hash_state = __insn_crc_32_32(hash_state, > 0xFFFFFFFF); \ > - hash_state = __insn_crc_32_32(hash_state, > 0xFEFDFCF5); \ > - (uint32_t)hash_state; \ > - }) > - > -#elif defined(__arm__) || defined(__aarch64__) > - > -#define PLATFORM_HASH_STATE uint32_t > - > -static inline uint32_t __crc32w(uint32_t crc, uint32_t value) > -{ > - __asm__("crc32w %w[c], %w[c], > %w[v]":[c]"+r"(crc):[v]"r"(value)); > - return crc; > -} > - > -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ > - ({ \ > - hash_state = 0xFEFDFCF5; \ > - hash_state = __crc32w(hash_state, name_len); \ > - }) > - > -#define PLATFORM_HASH32(hash_state, name_word) \ > - ({ \ > - __crc32w(hash_state, name_word); \ > - }) > - > -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ > - ({ \ > - hash_state = __crc32w(hash_state, kind); \ > - hash_state = __crc32w(hash_state, 0xFFFFFFFF); \ > - hash_state = __crc32w(hash_state, 0xFEFDFCF5); \ > - (uint32_t)hash_state; \ > - }) > - > -#else > -#error "Need to define PLATFORM_DEPENDENT_HASH32 macro" > -#endif > - > typedef struct name_tbl_entry_s name_tbl_entry_t; > /* It is important for most platforms that the following > struct fit within > @@ -227,7 +93,7 @@ typedef struct { > * NOT zero then this values points to a (linked list of) > name_table_entry_t > * records AND the bottom 6 bits are the length of this list. > */ > -typedef uint64_t hash_tbl_entry_t; > +typedef uintptr_t hash_tbl_entry_t; > typedef struct { > hash_tbl_entry_t hash_entries[SECONDARY_HASH_TBL_SIZE]; > @@ -275,53 +141,7 @@ static void aligned_free(void *mem_ptr) > static uint32_t hash_name_and_kind(const char *name, > uint8_t name_kind) > { > - PLATFORM_HASH_STATE hash_state; > - uint32_t name_len, name_word, hash_value; > - uint32_t bytes[4]; > - > - name_len = strlen(name); > - PLATFORM_HASH32_INIT(hash_state, name_len); > - > - while (4 <= name_len) { > - /* Get the next four characters. Note that > endianness doesn't > - * matter here! Also note that this assumes > that there is > - * either no alignment loading restrictions OR > that name is > - * 32-bit aligned. Shouldn't be too hard to > add code to deal > - * with the case when this assumption is false. > - */ > - /* name_word = *((uint32_t *)name); */ > - bytes[0] = name[0]; > - bytes[1] = name[1]; > - bytes[2] = name[2]; > - bytes[3] = name[3]; > - name_word = (bytes[3] << 24) | (bytes[2] << 16) | > - (bytes[1] << 8) | bytes[0]; > - PLATFORM_HASH32(hash_state, name_word); > - > - name_len -= 4; > - name += 4; > - } > - > - if (name_len != 0) { > - name_word = 0; > - > - if (2 <= name_len) { > - /* name_word = (uint32_t)*((uint16_t > *)name); */ > - bytes[0] = name[0]; > - bytes[1] = name[1]; > - name_word |= (bytes[1] << 8) | bytes[0]; > - name_len -= 2; > - name += 2; > - } > - > - if (name_len == 1) > - name_word |= ((uint32_t)*name) << 16; > - > - PLATFORM_HASH32(hash_state, name_word); > - } > - > - hash_value = PLATFORM_HASH32_FINISH(hash_state, > name_kind); > - return hash_value; > + return odp_hash_crc32c(name, strlen(name), name_kind); > } > static uint32_t linked_list_len(name_tbl_entry_t > *name_tbl_entry) > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
./build-all.sh for me insists that crc32w isn't supported on ARM, despite the fact that the patch removes those references. So it looks like the check-odp stuff is still flagging errors with the broken api-next rather than testing the patch that fixes these. It's confusing trying to see what check-odp is really doing here. On Mon, Dec 7, 2015 at 12:22 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/07/2015 20:14, Bill Fischofer wrote: > >> v1 should not have that problem. What error do you get when trying >> that? check-odp with M32_ON_64 works fine for me. >> > > export GIT_ODP=/opt/Linaro/odp2.git > export CLEANUP=1 > export GIT_BRANCH=`(cd $GIT_ODP; git symbolic-ref HEAD | cut -d / -f 3)` > ./build-all.sh > > > >> On Mon, Dec 7, 2015 at 9:01 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> shift still is not fixed: >> >> odp_name_table.c:186:4: error: right shift count >= width of type >> [-Werror] >> if ((hash_tbl_entry >> 48) == 0x7FFF) >> ^ >> odp_name_table.c:188:4: error: right shift count >= width of type >> [-Werror] >> else if ((hash_tbl_entry >> 48) == 0) >> >> >> >> On 12/07/2015 14:09, Bill Fischofer wrote: >> >> Change the internal hash_name_and_kind() function to eliminate >> the use >> of architecture-specific hash instructions. This eliminates >> build issues >> surrounding ARM variants. Future optimizations will use the >> arch directory >> consistent with other ODP modules. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> >> >> --- >> platform/linux-generic/odp_name_table.c | 184 >> +------------------------------- >> 1 file changed, 2 insertions(+), 182 deletions(-) >> >> diff --git a/platform/linux-generic/odp_name_table.c >> b/platform/linux-generic/odp_name_table.c >> index 10a760e..b4a9081 100644 >> --- a/platform/linux-generic/odp_name_table.c >> +++ b/platform/linux-generic/odp_name_table.c >> @@ -48,140 +48,6 @@ >> #define SECONDARY_HASH_HISTO_PRINT 1 >> - /* #define USE_AES */ >> - >> -#if defined __x86_64__ || defined __i386__ >> - >> -#ifdef USE_AES >> - >> -typedef long long int v2di __attribute__((vector_size(16))); >> - >> -static const v2di HASH_CONST1 = { 0x123456, 0xFEBCDA383 }; >> -static const v2di HASH_CONST2 = { 0x493BA3F689, 0x102F5D73A8C }; >> - >> -#define PLATFORM_HASH_STATE v2di >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = HASH_CONST1; \ >> - hash_state[0] ^= name_len; \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - v2di data; \ >> - \ >> - data[0] = name_word; \ >> - data[1] = name_word << 1; >> \ >> - hash_state = >> __builtin_ia32_aesenc128(hash_state, data); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) >> \ >> - ({ \ >> - uint64_t result; \ >> - v2di data; \ >> - \ >> - data[0] = name_kind; \ >> - data[1] = name_kind << 7; >> \ >> - hash_state = >> __builtin_ia32_aesenc128(hash_state, data); \ >> - hash_state = >> __builtin_ia32_aesenc128(hash_state, \ >> - HASH_CONST2); \ >> - hash_state = >> __builtin_ia32_aesenc128(hash_state, \ >> - HASH_CONST1); \ >> - result = (uint64_t)hash_state[0] ^ >> hash_state[1]; \ >> - result = result ^ result >> 32; >> \ >> - (uint32_t)result; \ >> - }) >> - >> -#else >> - >> -#define PLATFORM_HASH_STATE uint64_t >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = (uint64_t)name_len; \ >> - hash_state |= hash_state << 8; \ >> - hash_state |= hash_state << 16; \ >> - hash_state |= hash_state << 32; \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - uint64_t temp; \ >> - \ >> - temp = ((uint64_t)name_word) * >> 0xFEFDFCF5; \ >> - hash_state = hash_state * 0xFF; >> \ >> - hash_state ^= temp ^ (uint64_t)name_word; >> \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state ^= (((uint32_t)kind) << 13); \ >> - hash_state = hash_state * 0xFEFDFCF5; \ >> - hash_state = hash_state ^ hash_state >> 32; \ >> - hash_state = hash_state % 0xFEEDDCCBBAA1; \ >> - hash_state = hash_state ^ hash_state >> 32; \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#endif >> - >> -#elif defined(__tile_gx__) >> - >> -#define PLATFORM_HASH_STATE uint32_t >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = 0xFEFDFCF5; \ >> - hash_state = __insn_crc_32_32(hash_state, >> name_len); \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - hash_state = __insn_crc_32_32(hash_state, >> name_word); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state = __insn_crc_32_32(hash_state, >> kind); \ >> - hash_state = __insn_crc_32_32(hash_state, >> 0xFFFFFFFF); \ >> - hash_state = __insn_crc_32_32(hash_state, >> 0xFEFDFCF5); \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#elif defined(__arm__) || defined(__aarch64__) >> - >> -#define PLATFORM_HASH_STATE uint32_t >> - >> -static inline uint32_t __crc32w(uint32_t crc, uint32_t value) >> -{ >> - __asm__("crc32w %w[c], %w[c], >> %w[v]":[c]"+r"(crc):[v]"r"(value)); >> - return crc; >> -} >> - >> -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ >> - ({ \ >> - hash_state = 0xFEFDFCF5; \ >> - hash_state = __crc32w(hash_state, name_len); \ >> - }) >> - >> -#define PLATFORM_HASH32(hash_state, name_word) \ >> - ({ \ >> - __crc32w(hash_state, name_word); \ >> - }) >> - >> -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ >> - ({ \ >> - hash_state = __crc32w(hash_state, kind); \ >> - hash_state = __crc32w(hash_state, 0xFFFFFFFF); \ >> - hash_state = __crc32w(hash_state, 0xFEFDFCF5); \ >> - (uint32_t)hash_state; \ >> - }) >> - >> -#else >> -#error "Need to define PLATFORM_DEPENDENT_HASH32 macro" >> -#endif >> - >> typedef struct name_tbl_entry_s name_tbl_entry_t; >> /* It is important for most platforms that the following >> struct fit within >> @@ -227,7 +93,7 @@ typedef struct { >> * NOT zero then this values points to a (linked list of) >> name_table_entry_t >> * records AND the bottom 6 bits are the length of this list. >> */ >> -typedef uint64_t hash_tbl_entry_t; >> +typedef uintptr_t hash_tbl_entry_t; >> typedef struct { >> hash_tbl_entry_t hash_entries[SECONDARY_HASH_TBL_SIZE]; >> @@ -275,53 +141,7 @@ static void aligned_free(void *mem_ptr) >> static uint32_t hash_name_and_kind(const char *name, >> uint8_t name_kind) >> { >> - PLATFORM_HASH_STATE hash_state; >> - uint32_t name_len, name_word, hash_value; >> - uint32_t bytes[4]; >> - >> - name_len = strlen(name); >> - PLATFORM_HASH32_INIT(hash_state, name_len); >> - >> - while (4 <= name_len) { >> - /* Get the next four characters. Note that >> endianness doesn't >> - * matter here! Also note that this assumes >> that there is >> - * either no alignment loading restrictions OR >> that name is >> - * 32-bit aligned. Shouldn't be too hard to >> add code to deal >> - * with the case when this assumption is false. >> - */ >> - /* name_word = *((uint32_t *)name); */ >> - bytes[0] = name[0]; >> - bytes[1] = name[1]; >> - bytes[2] = name[2]; >> - bytes[3] = name[3]; >> - name_word = (bytes[3] << 24) | (bytes[2] << 16) | >> - (bytes[1] << 8) | bytes[0]; >> - PLATFORM_HASH32(hash_state, name_word); >> - >> - name_len -= 4; >> - name += 4; >> - } >> - >> - if (name_len != 0) { >> - name_word = 0; >> - >> - if (2 <= name_len) { >> - /* name_word = (uint32_t)*((uint16_t >> *)name); */ >> - bytes[0] = name[0]; >> - bytes[1] = name[1]; >> - name_word |= (bytes[1] << 8) | bytes[0]; >> - name_len -= 2; >> - name += 2; >> - } >> - >> - if (name_len == 1) >> - name_word |= ((uint32_t)*name) << 16; >> - >> - PLATFORM_HASH32(hash_state, name_word); >> - } >> - >> - hash_value = PLATFORM_HASH32_FINISH(hash_state, >> name_kind); >> - return hash_value; >> + return odp_hash_crc32c(name, strlen(name), name_kind); >> } >> static uint32_t linked_list_len(name_tbl_entry_t >> *name_tbl_entry) >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
diff --git a/platform/linux-generic/odp_name_table.c b/platform/linux-generic/odp_name_table.c index 10a760e..b4a9081 100644 --- a/platform/linux-generic/odp_name_table.c +++ b/platform/linux-generic/odp_name_table.c @@ -48,140 +48,6 @@ #define SECONDARY_HASH_HISTO_PRINT 1 - /* #define USE_AES */ - -#if defined __x86_64__ || defined __i386__ - -#ifdef USE_AES - -typedef long long int v2di __attribute__((vector_size(16))); - -static const v2di HASH_CONST1 = { 0x123456, 0xFEBCDA383 }; -static const v2di HASH_CONST2 = { 0x493BA3F689, 0x102F5D73A8C }; - -#define PLATFORM_HASH_STATE v2di - -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ - ({ \ - hash_state = HASH_CONST1; \ - hash_state[0] ^= name_len; \ - }) - -#define PLATFORM_HASH32(hash_state, name_word) \ - ({ \ - v2di data; \ - \ - data[0] = name_word; \ - data[1] = name_word << 1; \ - hash_state = __builtin_ia32_aesenc128(hash_state, data); \ - }) - -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ - ({ \ - uint64_t result; \ - v2di data; \ - \ - data[0] = name_kind; \ - data[1] = name_kind << 7; \ - hash_state = __builtin_ia32_aesenc128(hash_state, data); \ - hash_state = __builtin_ia32_aesenc128(hash_state, \ - HASH_CONST2); \ - hash_state = __builtin_ia32_aesenc128(hash_state, \ - HASH_CONST1); \ - result = (uint64_t)hash_state[0] ^ hash_state[1]; \ - result = result ^ result >> 32; \ - (uint32_t)result; \ - }) - -#else - -#define PLATFORM_HASH_STATE uint64_t - -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ - ({ \ - hash_state = (uint64_t)name_len; \ - hash_state |= hash_state << 8; \ - hash_state |= hash_state << 16; \ - hash_state |= hash_state << 32; \ - }) - -#define PLATFORM_HASH32(hash_state, name_word) \ - ({ \ - uint64_t temp; \ - \ - temp = ((uint64_t)name_word) * 0xFEFDFCF5; \ - hash_state = hash_state * 0xFF; \ - hash_state ^= temp ^ (uint64_t)name_word; \ - }) - -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ - ({ \ - hash_state ^= (((uint32_t)kind) << 13); \ - hash_state = hash_state * 0xFEFDFCF5; \ - hash_state = hash_state ^ hash_state >> 32; \ - hash_state = hash_state % 0xFEEDDCCBBAA1; \ - hash_state = hash_state ^ hash_state >> 32; \ - (uint32_t)hash_state; \ - }) - -#endif - -#elif defined(__tile_gx__) - -#define PLATFORM_HASH_STATE uint32_t - -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ - ({ \ - hash_state = 0xFEFDFCF5; \ - hash_state = __insn_crc_32_32(hash_state, name_len); \ - }) - -#define PLATFORM_HASH32(hash_state, name_word) \ - ({ \ - hash_state = __insn_crc_32_32(hash_state, name_word); \ - }) - -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ - ({ \ - hash_state = __insn_crc_32_32(hash_state, kind); \ - hash_state = __insn_crc_32_32(hash_state, 0xFFFFFFFF); \ - hash_state = __insn_crc_32_32(hash_state, 0xFEFDFCF5); \ - (uint32_t)hash_state; \ - }) - -#elif defined(__arm__) || defined(__aarch64__) - -#define PLATFORM_HASH_STATE uint32_t - -static inline uint32_t __crc32w(uint32_t crc, uint32_t value) -{ - __asm__("crc32w %w[c], %w[c], %w[v]":[c]"+r"(crc):[v]"r"(value)); - return crc; -} - -#define PLATFORM_HASH32_INIT(hash_state, name_len) \ - ({ \ - hash_state = 0xFEFDFCF5; \ - hash_state = __crc32w(hash_state, name_len); \ - }) - -#define PLATFORM_HASH32(hash_state, name_word) \ - ({ \ - __crc32w(hash_state, name_word); \ - }) - -#define PLATFORM_HASH32_FINISH(hash_state, kind) \ - ({ \ - hash_state = __crc32w(hash_state, kind); \ - hash_state = __crc32w(hash_state, 0xFFFFFFFF); \ - hash_state = __crc32w(hash_state, 0xFEFDFCF5); \ - (uint32_t)hash_state; \ - }) - -#else -#error "Need to define PLATFORM_DEPENDENT_HASH32 macro" -#endif - typedef struct name_tbl_entry_s name_tbl_entry_t; /* It is important for most platforms that the following struct fit within @@ -227,7 +93,7 @@ typedef struct { * NOT zero then this values points to a (linked list of) name_table_entry_t * records AND the bottom 6 bits are the length of this list. */ -typedef uint64_t hash_tbl_entry_t; +typedef uintptr_t hash_tbl_entry_t; typedef struct { hash_tbl_entry_t hash_entries[SECONDARY_HASH_TBL_SIZE]; @@ -275,53 +141,7 @@ static void aligned_free(void *mem_ptr) static uint32_t hash_name_and_kind(const char *name, uint8_t name_kind) { - PLATFORM_HASH_STATE hash_state; - uint32_t name_len, name_word, hash_value; - uint32_t bytes[4]; - - name_len = strlen(name); - PLATFORM_HASH32_INIT(hash_state, name_len); - - while (4 <= name_len) { - /* Get the next four characters. Note that endianness doesn't - * matter here! Also note that this assumes that there is - * either no alignment loading restrictions OR that name is - * 32-bit aligned. Shouldn't be too hard to add code to deal - * with the case when this assumption is false. - */ - /* name_word = *((uint32_t *)name); */ - bytes[0] = name[0]; - bytes[1] = name[1]; - bytes[2] = name[2]; - bytes[3] = name[3]; - name_word = (bytes[3] << 24) | (bytes[2] << 16) | - (bytes[1] << 8) | bytes[0]; - PLATFORM_HASH32(hash_state, name_word); - - name_len -= 4; - name += 4; - } - - if (name_len != 0) { - name_word = 0; - - if (2 <= name_len) { - /* name_word = (uint32_t)*((uint16_t *)name); */ - bytes[0] = name[0]; - bytes[1] = name[1]; - name_word |= (bytes[1] << 8) | bytes[0]; - name_len -= 2; - name += 2; - } - - if (name_len == 1) - name_word |= ((uint32_t)*name) << 16; - - PLATFORM_HASH32(hash_state, name_word); - } - - hash_value = PLATFORM_HASH32_FINISH(hash_state, name_kind); - return hash_value; + return odp_hash_crc32c(name, strlen(name), name_kind); } static uint32_t linked_list_len(name_tbl_entry_t *name_tbl_entry)
Change the internal hash_name_and_kind() function to eliminate the use of architecture-specific hash instructions. This eliminates build issues surrounding ARM variants. Future optimizations will use the arch directory consistent with other ODP modules. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/odp_name_table.c | 184 +------------------------------- 1 file changed, 2 insertions(+), 182 deletions(-)