RE: [V2] Adding new CRC32C implementation for IBM S390X

From: Eduard Stefes <Eduard(dot)Stefes(at)ibm(dot)com>
To: "johncnaylorls(at)gmail(dot)com" <johncnaylorls(at)gmail(dot)com>
Cc: "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "brueckner(at)linux(dot)ibm(dot)com" <brueckner(at)linux(dot)ibm(dot)com>, "iii(at)linux(dot)ibm(dot)com" <iii(at)linux(dot)ibm(dot)com>
Subject: RE: [V2] Adding new CRC32C implementation for IBM S390X
Date: 2025-07-02 08:27:46
Message-ID: 63f02b897e6604b21a98c7f96bb9c2adffac7ee0.camel@ibm.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On Wed, 2025-06-11 at 13:48 +0700, John Naylor wrote:
> Hi Eduard, thanks for the update. Note, it's not necessary to change
> the thread subject, and in fact I came very close to missing this
> email entirely.

Sorry for the confusion.

> Secondly, I haven't seen a response about the copyright. I'm
> referring
> to this part in particular:
>
> + * Copyright (c) 2025, International Business Machines (IBM)
>
> I shared this link in my first reply:
>
>
> It says, in part:
>
> "May I add my own copyright notice where appropriate?
>
> No, please don't. We like to keep the legal information short and
> crisp. Additionally, we've heard that could possibly pose problems
> for
> corporate users."
>

I had to talk to some people about this and get their ok. I'll remove
the copyright text.

> > - added gcc 14-14.2 as known broken compiler (bug was fixed with
> > 14.3)
>
> Why do we need to mark this as broken? In my research, it caused
> compiler errors with those versions -- that itself should cause
> fallback to sb8.
>
> On that note, I'm now wondering if clang compiles and links
> successfully but the program is broken? Or does it fail to compile?
> If
> the latter, we should treat them the same: there is no need for
> "known
> broken versions" in the configure checks, as it's just a matter of
> documentation.
>

Yes in all cases the compilation will fail and we will fall back to
sb8. However personally I think the user should get feedback of *why*
something fails. I'll remove the check and document the broken compiler
versions.

> > - create dependency to getauxval in configure, so we don't compile
> > the
> > code if we won't be able to detect the cpu extension at runtime
>
> That's just unnecessary clutter, and we don't do that anywhere else.
> We already have the HAVE_GETAUXVAL symbol to guard the runtime check.

Noted. I'll remove it.

> As I alluded to before, I'm not in favor of having both direct-call
> and runtime-check paths here. The reason x86 and Arm have it is
> because their hardware support works on any length input. Is there
> any
> reason to expect auxv.h to be unavailable?

I tried to find a reason but did not find any. So I'll remove it.

> Also, lately we have been moving away from having separate *choose.c
> files, since they add complexity to the build system. I'd suggest
> looking at src/port/pg_popcount_aarch64.c -- the file is compiled
> unconditionally but inside it has the appropriate #ifdef's as well as
> the choice function. See how it handles auxv.h as well.

I'll change that.

> Your tests demonstrated improvement with 32 bytes and above, and
> nothing less than 31 makes sense as a minimum because of the 16-byte
> alignment requirement. I've mentioned that we don't want the 20-byte
> WAL header computation to have any more overhead than it does now.

Sorry, yes that's true I'll change that back.

--
Eduard Stefes <eduard(dot)stefes(at)ibm(dot)com>

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrei Lepikhov 2025-07-02 09:01:39 Re: Memoize ANTI and SEMI JOIN inner
Previous Message Zhijie Hou (Fujitsu) 2025-07-02 08:16:01 RE: Report replica identity in pg_publication_tables