Обсуждение: [PATCH] Add loongarch native checksum implementation.

Hi,

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


See:

[1]: 
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#crc-check-instructions
[2]: 
https://gcc.gnu.org/onlinedocs/gcc/LoongArch-Base-Built-in-Functions.html
[3]: 
https://github.com/llvm/llvm-project/blob/release/16.x/clang/include/clang/Basic/BuiltinsLoongArch.def#L36-L39

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

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

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

Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, it has gcc-8.3 on it), i am trying to
testbuildfarm on it, when i edit build-farm.conf it seems to need animal and secret to connect the buildfarm server. 
then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and registered the buildfarm  a week ago, but
didn'treceive any response. so what else i need to do next. 



> -----Original Messages-----
> From: "YANG Xudong" <yangxudong@ymatrix.cn>
> Send time:Wednesday, 07/05/2023 10:15:51
> To: "John Naylor" <john.naylor@enterprisedb.com>
> Cc: pgsql-hackers@lists.postgresql.org, wengyanqing@ymatrix.cn, wanghao@ymatrix.cn
> Subject: Re: [PATCH] Add loongarch native checksum implementation.
>
> 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 <yangxudong@ymatrix.cn
> >> <mailto:yangxudong@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>
>


本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。

This email and its attachments contain confidential information from Loongson Technology , which is intended only for
theperson or entity whose address is listed above. Any use of the information contained herein in any way (including,
butnot limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended
recipient(s)is prohibited. If you receive this email in error, please notify the sender by phone or email immediately
anddelete it.  

> On 6 Jul 2023, at 09:14, huchangqi <huchangqi@loongson.cn> wrote:
>
> Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, it has gcc-8.3 on it), i am trying to
testbuildfarm on it, when i edit build-farm.conf it seems to need animal and secret to connect the buildfarm server. 
> then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and registered the buildfarm  a week ago,
butdidn't receive any response. so what else i need to do next. 

Thanks for volunteering a buildfarm animal!  The registration is probably just
pending due to it being summer and things slow down, but I've added Andrew
Dunstan who is the Buildfarm expert on CC:.

--
Daniel Gustafsson

On 2023/7/6 15:14, huchangqi wrote:
> Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, it has gcc-8.3 on it), i am trying to
testbuildfarm on it, when i edit build-farm.conf it seems to need animal and secret to connect the buildfarm server.
 
> then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and registered the buildfarm  a week ago,
butdidn't receive any response. so what else i need to do next.
 
> 
> 

Is it possible to provide a build farm instance for new world ABI of 
loongarch also by loongson? It will be really appreciated.

Thanks!

> 
>> -----Original Messages-----
>> From: "YANG Xudong" <yangxudong@ymatrix.cn>
>> Send time:Wednesday, 07/05/2023 10:15:51
>> To: "John Naylor" <john.naylor@enterprisedb.com>
>> Cc: pgsql-hackers@lists.postgresql.org, wengyanqing@ymatrix.cn, wanghao@ymatrix.cn
>> Subject: Re: [PATCH] Add loongarch native checksum implementation.
>>
>> 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 <yangxudong@ymatrix.cn
>>>> <mailto:yangxudong@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>
>>
> 
> 
>
本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。
> This email and its attachments contain confidential information from Loongson Technology , which is intended only for
theperson or entity whose address is listed above. Any use of the information contained herein in any way (including,
butnot limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended
recipient(s)is prohibited. If you receive this email in error, please notify the sender by phone or email immediately
anddelete it.
 

-------------------- Start of forwarded message --------------------
From: huchangqi <huchangqi@loongson.cn>
To: YANG Xudong <yangxudong@ymatrix.cn>
Subject: Re: [PATCH] Add loongarch native checksum implementation.
Date: Tue, 25 Jul 2023 15:51:43 +0800

Both cisticola and nuthatch are on the buildfarm now。

cisticola is "old world ABI".
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=cisticola&br=HEAD

nuthatch is "new world ABI".
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=nuthatch&br=HEAD

----------
Best regards,
huchangqi

YANG Xudong <yangxudong@ymatrix.cn> writes:

> On 2023/7/6 15:14, huchangqi wrote:
>> Hi, i have a loongarch machine runing Loongnix-server (based on redhat/centos, it has gcc-8.3 on it), i am trying to
testbuildfarm on it, when i edit build-farm.conf it seems to need animal and secret to connect the buildfarm server. 
>> then i go to  https://buildfarm.postgresql.org/cgi-bin/register-form.pl, and registered the buildfarm  a week ago,
butdidn't receive any response. so what else i need to do next. 
>>
>>
>
> Is it possible to provide a build farm instance for new world ABI of
> loongarch also by loongson? It will be really appreciated.
>
> Thanks!
>
>>
>>> -----Original Messages-----
>>> From: "YANG Xudong" <yangxudong@ymatrix.cn>
>>> Send time:Wednesday, 07/05/2023 10:15:51
>>> To: "John Naylor" <john.naylor@enterprisedb.com>
>>> Cc: pgsql-hackers@lists.postgresql.org, wengyanqing@ymatrix.cn, wanghao@ymatrix.cn
>>> Subject: Re: [PATCH] Add loongarch native checksum implementation.
>>>
>>> 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 <yangxudong@ymatrix.cn
>>>>> <mailto:yangxudong@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>
>>>
>>
>>
>>
本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。
>> This email and its attachments contain confidential information from Loongson Technology , which is intended only
forthe person or entity whose address is listed above. Any use of the information contained herein in any way
(including,but not limited to, total or partial disclosure, reproduction or dissemination) by persons other than the
intendedrecipient(s) is prohibited. If you receive this email in error, please notify the sender by phone or email
immediatelyand delete it. 
-------------------- End of forwarded message --------------------

Many thanks to huchangqi. Now we have loongarch64 support for both old 
world ABI and new world ABI on the buildfarm!

-------- Forwarded Message --------
Subject: Re: [PATCH] Add loongarch native checksum implementation.
Date: Tue, 25 Jul 2023 15:51:43 +0800
From: huchangqi <huchangqi@loongson.cn>
To: YANG Xudong <yangxudong@ymatrix.cn>

Both cisticola and nuthatch are on the buildfarm now。

cisticola is "old world ABI".
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=cisticola&br=HEAD

nuthatch is "new world ABI".
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=nuthatch&br=HEAD

----------
Best regards,
huchangqi


On 2023/7/5 15:11, John Naylor wrote:
 >
 > Before I look at this again: Are there any objections to another CRC
 > implementation for the reason of having no buildfarm member?

It is possible to try this patch on buildfarm now, I guess?

On Wed, Jul 05, 2023 at 02:11:02PM +0700, John Naylor wrote:
> Also, please don't top-post (which means: quoting an entire message, with
> new text at the top) -- it clutters our archives.
>
> Before I look at this again: Are there any objections to another CRC
> implementation for the reason of having no buildfarm member?

The performance numbers presented upthread for the CRC computations
are kind of nice in this environment, but honestly I have no idea how
much this architecture is used.  Perhaps that's only something in
China?  I am not seeing much activity around that in Japan, for
instance, and that's really close.

Anyway, based on today's state of the buildfarm, we have a buildfarm
member named cisticola that should be able to test this new CRC
implementation, so I see no problem in applying this stuff now if you
think it is in good shape.
--
Michael

On Wed, Jul 26, 2023 at 12:16:28PM +0900, Michael Paquier wrote:
> On Wed, Jul 05, 2023 at 02:11:02PM +0700, John Naylor wrote:
>> Before I look at this again: Are there any objections to another CRC
>> implementation for the reason of having no buildfarm member?
>
> [ ... ] 
> 
> Anyway, based on today's state of the buildfarm, we have a buildfarm
> member named cisticola that should be able to test this new CRC
> implementation, so I see no problem in applying this stuff now if you
> think it is in good shape.

IMHO we should strive to maintain buildfarm coverage for all the
instrinsics used within Postgres, if for no other reason than to ensure
future changes do not break those platforms.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 2023/7/26 11:16, Michael Paquier wrote> The performance numbers 
presented upthread for the CRC computations
> are kind of nice in this environment, but honestly I have no idea how
> much this architecture is used.  Perhaps that's only something in
> China?  I am not seeing much activity around that in Japan, for
> instance, and that's really close.

The architecture is pretty new (to open source ecosystem). The support 
of it in Linux kernel and GCC were released last year.

Here is a site about the status of major open source project support of 
it. (Chinese only)

https://areweloongyet.com/

Thanks for the comment. I have updated the patch to v3. Please have a look.


On 2023/8/7 19:01, John Naylor wrote:
> 
> On Fri, Jun 16, 2023 at 8:28 AM YANG Xudong <yangxudong@ymatrix.cn 
> <mailto:yangxudong@ymatrix.cn>> wrote:
>  > > +# 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.
> 
> (Looking again at v2)
> 
> The compilation test is found in c-compiler.m4, which still has all 
> logic for CFLAGS_CRC, including saving and restoring the old CFLAGS. Can 
> this also be simplified?

Fixed the function in c-compiler.m4 by removing the function argument 
and the logic of handling CFLAGS and CFLAGS_CRC.


> 
> I diff'd pg_crc32c_loongarch.c with the current other files, and found 
> it is structurally the same as the Arm implementation. That's logical if 
> memory alignment is important.
> 
>    /*
> - * ARMv8 doesn't require alignment, but aligned memory access is
> - * significantly faster. Process leading bytes so that the loop below
> - * starts with a pointer aligned to eight bytes.
> + * Aligned memory access is significantly faster.
> + * Process leading bytes so that the loop below starts with a pointer 
> aligned to eight bytes.
> 
> Can you confirm the alignment requirement -- it's not clear what the 
> intention is since "doesn't require" wasn't carried over. Is there any 
> documentation (or even a report in some other context) about aligned vs 
> unaligned memory access performance?

It is in the official document that the alignment is not required.


https://github.com/loongson/la-softdev-convention/blob/master/la-softdev-convention.adoc#74-unaligned-memory-access-support


However, I found this patch in LKML that shows great performance gain 
when using aligned memory access similar to this patch.

https://lore.kernel.org/lkml/20230410115734.93365-1-wangrui@loongson.cn/

So I guess using aligned memory access is necessary and I have updated 
the comment in the code.


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

On 2023/8/8 14:38, John Naylor wrote:
> 
> It seems that platforms capable of running Postgres 
> only support 64 bit.

I think so.


>  > So I guess using aligned memory access is necessary and I have updated
>  > the comment in the code.
> 
> Okay, so it's not "necessary" in the sense that it's illegal, so I'm 
> thinking we can just re-use the Arm comment language, as in 0002.

Yes. I think it is similar to Arm.


> v4 0001 is the same as v3, but with a draft commit message. I will 
> squash and commit this week, unless there is additional feedback.

Looks good to me. Thanks for the additional patch.

On Thu, Aug 10, 2023 at 10:35 AM John Naylor
<john.naylor@enterprisedb.com> wrote:
>
> On Tue, Aug 8, 2023 at 2:22 PM YANG Xudong <yangxudong@ymatrix.cn> wrote:
> >
> > On 2023/8/8 14:38, John Naylor wrote:
> >
> > > v4 0001 is the same as v3, but with a draft commit message. I will
> > > squash and commit this week, unless there is additional feedback.
> >
> > Looks good to me. Thanks for the additional patch.
>
> I pushed this with another small comment change. Unfortunately, I didn't glance at the buildfarm beforehand -- it
seemsmany members are failing an isolation check added by commit fa2e87494, including both loongarch64 members. I'll
checkback periodically. 
>
> https://buildfarm.postgresql.org/cgi-bin/show_status.pl
>

In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
facing the below error:
Generating configuration headers...
undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.

Am I missing something or did the commit miss something?

--
With Regards,
Amit Kapila.

On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
> facing the below error:
> Generating configuration headers...
> undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
> 718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
>
> Am I missing something or did the commit miss something?

Yes, the commit has missed the addition of USE_LOONGARCH_CRC32C in
Solution.pm.  If you want to be consistent with pg_config.h.in, you
could add it just after USE_LLVM, for instance.
--
Michael

On Thu, Aug 10, 2023 at 5:07 PM John Naylor
<john.naylor@enterprisedb.com> wrote:
>
> On Thu, Aug 10, 2023 at 5:54 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Aug 10, 2023 at 03:56:37PM +0530, Amit Kapila wrote:
> > > In MSVC build, on doing: perl mkvcbuild.pl after this commit, I am
> > > facing the below error:
> > > Generating configuration headers...
> > > undefined symbol: USE_LOONGARCH_CRC32C at src/include/pg_config.h line
> > > 718 at ../postgresql/src/tools/msvc/Mkvcbuild.pm line 872.
> > >
> > > Am I missing something or did the commit miss something?
> >
> > Yes, the commit has missed the addition of USE_LOONGARCH_CRC32C in
> > Solution.pm.  If you want to be consistent with pg_config.h.in, you
> > could add it just after USE_LLVM, for instance.
>
> Oops, fixing now...
>

It is fixed now. Thanks!

--
With Regards,
Amit Kapila.