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>