Re: [PATCH] Add loongarch native checksum implementation.

Attached a new patch with fixes based on the comment below.

On 2023/6/13 18:26, John Naylor wrote:
>
> On Thu, Jun 8, 2023 at 12:24 PM YANG Xudong <yangxudong(at)ymatrix(dot)cn
> <mailto:yangxudong(at)ymatrix(dot)cn>> wrote:
> >
> > This patch tries to add loongarch native crc32 check with crcc.*
> > instructions to postgresql.
> >
> > The patch is tested on my Loongson 3A5000 machine with Loong Arch Linux
> > and GCC 13.1.0 / clang 16.0.0 with
> >
> > - default ./configure
> > - default meson setup
>
> I took a quick look at this, and it seems mostly in line with other
> architectures we support for CRC. I have a couple questions and comments:
>
> configure.ac <http://configure.ac>:
>
> +AC_SUBST(CFLAGS_CRC)
> > This seems to be an unnecessary copy-paste. I think we only need one,
> after all checks have run.
>

Removed the extra line.

> meson.build
>
> +  if cc.links(prog, name: '__builtin_loongarch_crcc_w_b_w,
> __builtin_loongarch_crcc_w_h_w, __builtin_loongarch_crcc_w_w_w, and
> __builtin_loongarch_crcc_w_d_w without -march=loongarch64',
> +      args: test_c_args)
> +    # Use LoongArch CRC instruction unconditionally
> +    cdata.set('USE_LOONGARCH_CRC32C', 1)
> +    have_optimized_crc = true
> +  elif cc.links(prog, name: '__builtin_loongarch_crcc_w_b_w,
> __builtin_loongarch_crcc_w_h_w, __builtin_loongarch_crcc_w_w_w, and
> __builtin_loongarch_crcc_w_d_w with -march=loongarch64',
> +      args: test_c_args + ['-march=loongarch64'])
> +    # Use LoongArch CRC instruction unconditionally
>
> 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.

> 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.

Also the output of make check is attached.

I run a simple test program to compare the performance of
pg_comp_crc32c_loongarch and pg_comp_crc32c_sb8 on my test machine. The
result is that pg_comp_crc32c_loongarch is over 2x faster than
pg_comp_crc32c_sb8.

> In the future, you may also consider running the buildfarm client on a
> machine dedicated for testing. That will give us quick feedback if some
> future new code doesn't work on this platform. More information here:
>
> https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto
> <https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto>
>

I will contact the loongson community
(https://github.com/loongson-community) to see if they are able to
provide some machine for buildfarm or not.

> --
> John Naylor
> EDB: http://www.enterprisedb.com <http://www.enterprisedb.com>

--
YANG Xudong