Is there any other comment?
If the patch looks OK, I would like to update its status to ready for
committer in the commitfest.
Thanks!
On 2023/6/16 09:28, YANG Xudong wrote:
Updated the patch based on the comments.
On 2023/6/15 18:30, John Naylor wrote:
On Wed, Jun 14, 2023 at 9:20 AM YANG Xudong <yangxud...@ymatrix.cn
<mailto: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.
Added v2
> > 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.
Removed these lines.
+# 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?
We don't need to set CFLAGS_CRC as commented. I have updated the
configure script to make it align with the logic in meson build script.
> > 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 <http://www.enterprisedb.com>
