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@ymatrix.cn 
> <mailto:yangxudong@ymatrix.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