On Tue, Jul 30, 2024 at 11:16 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 30/07/2024 00:50, Thomas Munro wrote:
> > On Wed, Jul 3, 2024 at 8:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Thomas Munro <thomas.munro@gmail.com> writes:
> > OK, <stdatomic.h> part on ice for now. Here's an update of the rest,
> > this time also removing the barrier fallbacks as discussed in the LTO
> > thread[1].
>
> Looks good to me.
Thanks. I'll wait just a bit longer to see if anyone else has comments.
> > Perhaps I'm missing something but I suspect we might be failing to
> > include arch-x86.h on that compiler when we should... maybe it needs
> > to detect _M_AMD64 too?
>
> Aha, yes I think that's it. Apparently, __x86_64__ is not defined on
> MSVC. To prove that, I added garbage to the "#ifdef __x86_64__" guarded
> block in atomics.h. The compilation passes on MSVC, but not on other
> platforms: https://cirrus-ci.com/build/6310061188841472.
>
> That means that we're not getting the x86-64 instructions in
> src/port/pg_crc32c_sse42.c on MSVC either.
>
> I think we should do:
>
> #ifdef _M_AMD64
> #define __x86_64__
> #endif
>
> somewhere, perhaps in src/include/port/win32.h.
Hmm. I had come up with the opposite solution, because we already
tested for _M_AMD64 explicitly elsewhere, and also I was thinking we
would back-patch, and I don't want to cause problems for external code
that thinks that __x86_64__ implies it can bust out some GCC inline
assembler or something. But I don't have a strong opinion, your idea
is certainly simpler to implement and I also wouldn't mind much if we
just fixed it in master only, for fear of subtle breakage...
Same problem probably exists for i386. I don't think CI, build farm
or the EDB packaging team do 32 bit Windows, so that makes it a little
hard to know if your blind code changes have broken or fixed
anything... on the other hand it's pretty simple...
I wondered if the pre-Meson system might have somehow defined
__x86_64__, but I'm not seeing it. Commit b64d92f1a56 explicitly
mentions that it was tested on MSVC, so I guess maybe it was just
always "working" but not quite taking the intended code paths? Funny
though, that code that calls _mm_pause() on AMD64 or the __asm thing
that only works on i386 doesn't look like blind code to me. Curious.
