On 23 January 2015 at 16:28, Steve Capper <steve.cap...@linaro.org> wrote: > Hi Yazen, > > This is looking good, just a few minor comments below. > > Cheers, > -- > Steve > > On Fri, Jan 23, 2015 at 09:13:42AM -0600, Yazen Ghannam wrote: >> ARMv8 defines a set of optional CRC32/CRC32C instructions. >> This patch defines an optimized function that uses these >> instructions when available rather than table-based lookup. >> Optimized function based on a Hadoop patch by Ed Nevill. >> >> Autotools updated to check for compiler support. >> Optimized function is selected at runtime based on HWCAP_CRC32. >> Added crc32c "performance" unit test and arch unit test. >> >> Tested on AMD Seattle. >> Passes all crc32c unit tests. >> Unit test shows ~4x performance increase versus sctp. >> >> Signed-off-by: Yazen Ghannam <yazen.ghan...@linaro.org> >> --- >> configure.ac | 1 + >> m4/ax_arm.m4 | 18 +++++++++++-- >> src/arch/arm.c | 2 ++ >> src/arch/arm.h | 1 + >> src/common/Makefile.am | 10 +++++++- >> src/common/crc32c.cc | 6 +++++ >> src/common/crc32c_aarch64.c | 58 >> ++++++++++++++++++++++++++++++++++++++++++ >> src/common/crc32c_aarch64.h | 25 ++++++++++++++++++ >> src/test/common/test_crc32c.cc | 11 ++++++++ >> src/test/test_arch.cc | 3 +++ >> 10 files changed, 132 insertions(+), 3 deletions(-) >> create mode 100644 src/common/crc32c_aarch64.c >> create mode 100644 src/common/crc32c_aarch64.h >> [...] >> diff --git a/src/test/test_arch.cc b/src/test/test_arch.cc >> index b129262..27cdd1d 100644 >> --- a/src/test/test_arch.cc >> +++ b/src/test/test_arch.cc >> @@ -50,6 +50,9 @@ TEST(Arch, all) >> expected = (strstr(flags, " neon ") || strstr(flags, " asimd ")) ? 1 : 0; >> EXPECT_EQ(expected, ceph_arch_neon); >> >> + expected = strstr(flags, " crc32 ") ? 1 : 0; >> + EXPECT_EQ(expected, ceph_arch_aarch64_crc32); >> + > > The test_arch.cc file looks very dangerous to me, it's strstr scanning > for CPU flags from /proc/cpuinfo with no regard for architecture. > (It is quite likely another chip in future introduces the crc32 CPU flag). > > I see it's trying to sanity check that the HW_CAPS logic matches up > with the cpuflags. Could you please surround the crc32 check with: > > #ifdef __arch64__ > #endif /* __arch64__ */ > > Just to pin it down to ARM. >
Any way we could use getauxval(AT_HWCAP) here? It has been around since before AArch64 glibc support stabilised, afaik, and parsing /proc/cpuinfo is one of the things we really get rid of. (Perhaps in a separate patch) -- Ard. _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
