Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.2] avoid redefining _mm_crc32_u64 which can lead to compile errors #242

Merged
merged 1 commit into from
Sep 27, 2022
Merged

[3.2] avoid redefining _mm_crc32_u64 which can lead to compile errors #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions libraries/libfc/src/crypto/city.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@

#if defined(__SSE4_2__) && defined(__x86_64__)
#include <nmmintrin.h>
#define MM_CRC32_U64I _mm_crc32_u64
#else
uint64_t _mm_crc32_u64(uint64_t a, uint64_t b );
uint64_t mm_crc32_u64(uint64_t a, uint64_t b );
#define MM_CRC32_U64I mm_crc32_u64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use FC_MM_CRC32_U64I instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to keep the same length to avoid touching a bunch of whitespace over in CHUNK() 😅

Since this define is local to a .cpp it doesn't seem like there is much risk of a clash? Possibly mm_crc32_u64() should be moved in to fc's namespace though?

Copy link
Member Author

@spoonincode spoonincode Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually if _mm_crc32_u64 is just placed in to a namespace I think that's safe. maybe that's the better choice 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice this was only in this one source file. I think it is fine as is.

#endif

namespace fc {
Expand Down Expand Up @@ -578,9 +580,9 @@ static void CityHashCrc256Long(const char *s, size_t len,
g += e; \
e += z; \
g += x; \
z = _mm_crc32_u64(z, b + g); \
y = _mm_crc32_u64(y, e + h); \
x = _mm_crc32_u64(x, f + a); \
z = MM_CRC32_U64I(z, b + g); \
y = MM_CRC32_U64I(y, e + h); \
x = MM_CRC32_U64I(x, f + a); \
e = Rotate(e, r); \
c += e; \
s += 40
Expand Down
4 changes: 2 additions & 2 deletions libraries/libfc/src/crypto/crc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ static const uint32_t crc_c[256] = {
#if !defined __SSE4_2__ || (defined __SSE4_2__ && !defined __x86_64__)


uint64_t _mm_crc32_u64(uint64_t a, uint64_t b )
uint64_t mm_crc32_u64(uint64_t a, uint64_t b )
{
return crc32cSlicingBy8(a, (unsigned char*)&b, sizeof(b));
}
Expand All @@ -617,7 +617,7 @@ int main( int argc, char** argv )
{
uint64_t f = 0x1234;
uint64_t a = 0x5678;
uint32_t f1 = _mm_crc32_u64(f, a);
uint32_t f1 = mm_crc32_u64(f, a);
uint32_t f4 = crc32cSlicingBy8(f, (unsigned char*)&a, sizeof(a));
std::cout<<std::hex<<f1<<"\n"<<f2<<"\n"<<f3<<"\n"<<f4<<"\n";
return 0;
Expand Down