Re: [PATCH] Add loongarch native checksum implementation.

On Tue, Aug 8, 2023 at 10:07 AM YANG Xudong <yangxudong@ymatrix.cn> wrote:

> On 2023/8/7 19:01, John Naylor wrote:

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

Looks good to me. It seems that platforms capable of running Postgres only
support 64 bit. If that ever changes, the compiler intrinsic test (with 8
byte CRC input) should still gate that well enough in autoconf, I believe,
so in v4 I added a comment to clarify this. The Meson build checks hostcpu
first for all platforms, and the patch is consistent with surrounding code.
In the attached 0002 addendum, I change a comment in configure.ac to
clarify "override" is referring to the runtime check for x86 and Arm, and
that LoongArch doesn't need one.

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

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.

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.

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