On Wed, Jun 14, 2023 at 9:20 AM YANG Xudong <yangxud...@ymatrix.cn> wrote: > > Attached a new patch with fixes based on the comment below.
Note: It's helpful to pass "-v" to git format-patch, to have different versions. > > For x86 and Arm, if it fails to link without an -march flag, we allow > > for a runtime check. The flags "-march=armv8-a+crc" and "-msse4.2" are > > for instructions not found on all platforms. The patch also checks both > > ways, and each one results in "Use LoongArch CRC instruction > > unconditionally". The -march flag here is general, not specific. In > > other words, if this only runs inside "+elif host_cpu == 'loongarch64'", > > why do we need both with -march and without? > > > > Removed the elif branch. Okay, since we've confirmed that no arch flag is necessary, some other places can be simplified: --- a/src/port/Makefile +++ b/src/port/Makefile @@ -98,6 +98,11 @@ pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC) pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC) pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC) +# all versions of pg_crc32c_loongarch.o need CFLAGS_CRC +pg_crc32c_loongarch.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_loongarch_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_loongarch_srv.o: CFLAGS+=$(CFLAGS_CRC) This was copy-and-pasted from platforms that use a runtime check, so should be unnecessary. +# If the intrinsics are supported, sets pgac_loongarch_crc32c_intrinsics, +# and CFLAGS_CRC. +# Check if __builtin_loongarch_crcc_* intrinsics can be used +# with the default compiler flags. +# CFLAGS_CRC is set if the extra flag is required. Same here -- it seems we don't need to set CFLAGS_CRC at all. Can you confirm? > > Also, I don't have a Loongarch machine for testing. Could you show that > > the instructions are found in the binary, maybe using objdump and grep? > > Or a performance test? > > > > The output of the objdump command `objdump -dS > ../postgres-build/tmp_install/usr/local/pgsql/bin/postgres | grep -B 30 > -A 10 crcc` is attached. Thanks for confirming. -- John Naylor EDB: http://www.enterprisedb.com