Re: [PATCH] Add loongarch native checksum implementation.

From: YANG Xudong <yangxudong(at)ymatrix(dot)cn>
To: John Naylor <john(dot)naylor(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, wengyanqing(at)ymatrix(dot)cn, wanghao(at)ymatrix(dot)cn
Subject: Re: [PATCH] Add loongarch native checksum implementation.
Date: 2023-06-16 01:28:07
Message-ID: 1b1094a4-cfcf-ed8b-4db2-c793af823414@ymatrix.cn
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 <yangxudong(at)ymatrix(dot)cn
> <mailto:yangxudong(at)ymatrix(dot)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>

Attachment Content-Type Size
v2-0001-Add-loongarch-native-checksum-implementation.patch text/plain 14.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2023-06-16 01:30:09 Re: add non-option reordering to in-tree getopt_long
Previous Message Amit Langote 2023-06-16 01:25:13 Re: Fix a typo in rewriteHandler.c