[PATCH] D32971: [scudo] CRC32 optimizations

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:78
     for (uptr i = 0; i < ARRAY_SIZE(HeaderHolder); i++)
       Crc = computeCRC32(Crc, HeaderHolder[i], HashType);
+#endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
----------------
alekseyshl wrote:
> Would it make sense to check algorithm once and duplicate the code here, instead of comparing on every loop iteration? More code, but since we're after the performance...
> 
>   if (HashType == CRC32Hardware) {
>     Crc = computeHardwareCRC32(Cookie, reinterpret_cast<uptr>(this));
>     for (uptr i = 0; i < ARRAY_SIZE(HeaderHolder); i++)
>       Crc = computeHardwareCRC32(Crc, HeaderHolder[i]);
>   } else {
>     Crc = computeSoftwareCRC32(Cookie, reinterpret_cast<uptr>(this));
>     for (uptr i = 0; i < ARRAY_SIZE(HeaderHolder); i++)
>       Crc = computeSoftwareCRC32(Crc, HeaderHolder[i]);
>   }
I gave this a try, with -O3, the generated code for `computeChecksum` ends up being identical with the initial version of the patch.


================
Comment at: lib/scudo/scudo_crc32.cpp:23
 }
 #endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
----------------
alekseyshl wrote:
> Please remind me, does it mean that computeHardwareCRC32 != 0 only when defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)? Or is it expected to come from other libraries too?
computeHardwareCRC32 != 0 if `defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)`, but it could also be defined by an external library, even though this is **not** the expectation. With full RELRO (`-Wl,-z,relro,-z,now`), if not defined at load, it can't be defined later.


https://reviews.llvm.org/D32971