Jim Apple has posted comments on this change. Change subject: IMPALA-3604: Clean up SSE handling ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/3227/2/be/src/util/sse-util.h File be/src/util/sse-util.h: Have you tested this version of Impala (when compiled on an sse4.2 machine) running on non-sse4.2 machines? Have you tried compiling this on non-sse4.2 machines? Have you looked at the generated code to check that the intrinsics aren't generating worse code? This has been known to happen. Line 94: __asm__ volatile ("pcmpestrm %5, %2, %1" The toolchain uses clang 3.8 now, so we can depend on that. Line 112: __attribute__((target("sse4.2"))) : static inline uint32_t SSE4_crc32_u8(uint32_t crc, uint8_t v) { : return _mm_crc32_u8(crc, v); : } : : __attribute__((target("sse4.2"))) : static inline uint32_t SSE4_crc32_u16(uint32_t crc, uint16_t v) { : return _mm_crc32_u16(crc, v); : } : : __attribute__((target("sse4.2"))) : static inline uint32_t SSE4_crc32_u32(uint32_t crc, uint32_t v) { : return _mm_crc32_u32(crc, v); : } : : __attribute__((target("sse4.2"))) : static inline uint32_t SSE4_crc32_u64(uint32_t crc, uint64_t v) { : return _mm_crc32_u64(crc, v); : } crc32 is it's own gcc target, I think. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html Line 132: sse4 popcnt is its own target, I think. -- To view, visit http://gerrit.cloudera.org:8080/3227 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id346187f3248922ec2a323548de4bdeaccd7a771 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
