Centralised architecture detection
Hi,
Catching up with the timing_clock_source thread (great work!), I saw
that Andres mentioned that it'd be nice to have PG_ARCH_X86 etc macros
to standardise our detection of CPUs[1]/messages/by-id/r5snevsnkyoifjqldu6gcssbnrnezpplq4ofcmekjfvzzu32dc@5rn26itd4ubr, and I remembered that I'd
already looked into that a couple of years ago and posted a patch...
and promptly forgot all about it. Then I remembered the surprising
discovery that motivated it[2]/messages/by-id/CA+hUKGKAf_i6w7hB_3pqZXQeqn+ixvY+CMps_n=mJ5HAatMjMw@mail.gmail.com, picked up in passing while working on
early versions of my <stdatomic.h> patch, that we still haven't done
anything about:
We are not detecting x86 in several places on Visual Studio, and the
one where we fail to include src/port/atomics/arch-x86.h can't be too
great for performance.
* pg_{read,write}_barrier() → pg_memory_barrier()!
* pg_spin_delay() → nothing!
* PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY → undefined
No Windows here but it's easy enough to confirm with CI: add #error to
arch-x86.h and you'll get a red MinGW task and a green Visual Studio
task. Presumably the Windows support was either written blind or
developed on MinGW.
Here's an update of my old patch. It just defines macros like this,
in c.h, though since then we gained port/pg_cpu.h, so perhaps it
belongs in there. Then we can deal with the underlying
compiler-defined macros in one central place. The names I came up
with were:
PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}
PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}_{32,64}
Lukas and John have both been doing similar sorts of things and may
have better ideas or patches, but I figured I should at least re-post
what I have.
I suppose we could also do something similar for PG_COMPILER_XXX and
PG_OS_XXX. I've made mistakes with those before too...
[1]: /messages/by-id/r5snevsnkyoifjqldu6gcssbnrnezpplq4ofcmekjfvzzu32dc@5rn26itd4ubr
[2]: /messages/by-id/CA+hUKGKAf_i6w7hB_3pqZXQeqn+ixvY+CMps_n=mJ5HAatMjMw@mail.gmail.com
Here's a better version (I'd omitted _M_AMD64 in the previous one
after testing something...). CC'ing Bryan who might be interested in
the effects of this stuff on Windows builds.
On Thu, Apr 9, 2026 at 8:02 AM Thomas Munro <thomas.munro@gmail.com> wrote:
Here's an update of my old patch. It just defines macros like this,
in c.h, though since then we gained port/pg_cpu.h, so perhaps it
belongs in there.
port/pg_cpu.h would seem like the natural place for something like
this, and I deliberately made that header's name not specific to one
architecture. But see below.
Lukas and John have both been doing similar sorts of things and may
have better ideas or patches, but I figured I should at least re-post
what I have.
From that thread, I think the final committed version ended up with
fewer places that cared about architecture macros, and that's why it
was left out. I for one was not motivated to continue that work, but I
don't see a reason not to, either. And as you said, this might help
avoid errors of omission going forward.
--- a/src/port/pg_crc32c_sse42.c
+++ b/src/port/pg_crc32c_sse42.c
@@ -39,7 +39,7 @@ pg_comp_crc32c_sse42(pg_crc32c crc, const void
*data, size_t len)
* and performance testing didn't show any performance gain from aligning
* the begin address.
*/
-#ifdef __x86_64__
+#ifdef PG_ARCH_X86_64
while (p + 8 <= pend)
That probably should have been "SIZEOF_VOID_P >= 8" to begin with.
Also, src/port/pg_cpu_x86.c currently has this hack:
#if defined(USE_SSE2) || defined(__i386__)
That's an awkward way of saying "x86 of any word size, but forget
about 32-bit MSVC because it won't get tested in the buildfarm", and
should probably use PG_ARCH_X86 from this patch. However, as currently
written, that would only work if the new macros were in c.h, to keep
the property that system headers come before (most) PG headers.
--
John Naylor
Amazon Web Services
Thomas Munro <thomas.munro@gmail.com> writes:
Instead of repeating compilers' architecture macros throughout the tree
and sometimes getting it wrong, let's detect them in one central place,
and define our own macros of the form:PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}
PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}_{32,64}
[...]
diff --git a/contrib/pgcrypto/crypt-blowfish.c b/contrib/pgcrypto/crypt-blowfish.c index 5a1b1e10091..9c4e02e428b 100644 --- a/contrib/pgcrypto/crypt-blowfish.c +++ b/contrib/pgcrypto/crypt-blowfish.c @@ -38,10 +38,10 @@ #include "px-crypt.h" #include "px.h"-#ifdef __i386__
+#if defined(PG_ARCH_X86_32)
#define BF_ASM 0 /* 1 */
#define BF_SCALE 1
-#elif defined(__x86_64__)
+#elif defined(PG_ARCH_X86_64)
#define BF_ASM 0
#define BF_SCALE 1
#else
These could be combined into a single #ifdef PG_ARCH_X86. Also, BF_ASM
has never been defined to anything but 0 since this file was added in
2001, and the _BF_body_r function it would call in that case has never
existed, so we could get rid of it (but that would be for a separate
patch).
- ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
Thomas Munro <thomas.munro@gmail.com> writes:
Instead of repeating compilers' architecture macros throughout the tree
and sometimes getting it wrong, let's detect them in one central place,
and define our own macros of the form:PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}
PG_ARCH_{ARM,LOONGARCH,MIPS,PPC,RISCV,S390,SPARC,X86}_{32,64}
Nathan Bossart reminded me of this thread after I'd independently
rediscovered the same thing [1]/messages/by-id/3035145.1780503430@sss.pgh.pa.us. I agree with standardizing on
just one spelling of these CPU-type macros. But I wonder why we
should invent our own instead of standardizing on gcc's spellings
(that is, __x86_64__ etc). The amount of code churn required for
this patch would drop drastically if we did it that way. And I
suspect it would be less likely that we'd need to fixup future patch
submissions than if we have a homegrown standard.
regards, tom lane
I wrote:
Nathan Bossart reminded me of this thread after I'd independently
rediscovered the same thing [1]. I agree with standardizing on
just one spelling of these CPU-type macros. But I wonder why we
should invent our own instead of standardizing on gcc's spellings
(that is, __x86_64__ etc). The amount of code churn required for
this patch would drop drastically if we did it that way. And I
suspect it would be less likely that we'd need to fixup future patch
submissions than if we have a homegrown standard.
Concretely, I'm imagining that we'd do more or less the attached in
c.h, and then the rest of the work would just be to remove the
not-very-large number of references to the alternative CPU symbols.
Note that I threw in an "#else #error" branch to ensure that we
successfully identify every architecture. Even if you don't like
this naming scheme, we should do that with the original patch too.
regards, tom lane
Attachments:
wip-standardize-macros-for-detecting-architectures.patchtext/x-diff; charset=us-ascii; name=wip-standardize-macros-for-detecting-architectures.patchDownload+53-1
On Wed, Jun 03, 2026 at 05:08:55PM -0400, Tom Lane wrote:
I wrote:
Nathan Bossart reminded me of this thread after I'd independently
rediscovered the same thing [1]. I agree with standardizing on
just one spelling of these CPU-type macros. But I wonder why we
should invent our own instead of standardizing on gcc's spellings
(that is, __x86_64__ etc). The amount of code churn required for
this patch would drop drastically if we did it that way. And I
suspect it would be less likely that we'd need to fixup future patch
submissions than if we have a homegrown standard.Concretely, I'm imagining that we'd do more or less the attached in
c.h, and then the rest of the work would just be to remove the
not-very-large number of references to the alternative CPU symbols.
Can a pre-processor make it an error for users to define __ macros?
I wrote:
Concretely, I'm imagining that we'd do more or less the attached in
c.h, and then the rest of the work would just be to remove the
not-very-large number of references to the alternative CPU symbols.
Here's a fleshed-out (and now actually lightly-tested) version
of that.
regards, tom lane