Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Substitution for MemoryBarrier() to avoid including <windows.h> #1199

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Substitution for MemoryBarrier() to avoid including <windows.h> #1199

merged 1 commit into from
Aug 12, 2024

Conversation

Epixu
Copy link
Contributor

@Epixu Epixu commented Jul 21, 2024

Preprocessor checks should probably be substituted with SIMDE_* alternatives, but I'm not very familiar with these, so it could use a review. #1193

@Epixu
Copy link
Contributor Author

Epixu commented Jul 21, 2024

In this failure it reached a case where no implementation was available, neither in the original, nor in the wine's winnt.h. Or I may have simply overlooked some of the preprocessor conditions. Is MemoryBarrier() in this case noop?

Edit: Nevermind, missed a branch:

#if defined(__aarch64__) || defined(_M_ARM64)
  #define simde_MemoryBarrier __faststorefence

@mr-c
Copy link
Collaborator

mr-c commented Jul 22, 2024

@Epixu
Copy link
Contributor Author

Epixu commented Jul 22, 2024

Yes, I'm aware, but my time budget went a bit constrained. I'll eventually finish it, but I'll appreciate any help. I think these are mostly technical mistakes. One of the previous runs had only one failing build (referenced via my previous comment with strikethrough)

simde/x86/sse.h Outdated
simde_InterlockedOr(&dummy, 0);
}
#else
#error "should simde_MemoryBarrier be defined as no-op in this case?"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for when SIMDE_X86_SSE_NO_NATIVE is defined on x86-64 systems; at least

@Epixu
Copy link
Contributor Author

Epixu commented Aug 12, 2024

Tests passed here, but all my x86 builds still failed, so you probably don't cover some of these cases in tests for MSVC:

  #elif defined(SIMDE_ARCH_X86) || defined(SIMDE_ARCH_AMD64) || defined(SIMDE_ARCH_E2K)
    #if !defined(SIMDE_X86_SSE_NO_NATIVE)
      #include <intrin.h>
    #endif

    HEDLEY_ALWAYS_INLINE
    void simde_MemoryBarrier(void) {
      #if defined(SIMDE_X86_SSE_NO_NATIVE)
         ((void)0); // intentionally no-op
      #elif defined(SIMDE_ARCH_AMD64)
        __faststorefence();
      #elif defined(SIMDE_ARCH_IA64)
        __mf();
      #else
        // This case is definitely not convered
        long Barrier;
        __asm { xchg Barrier, eax }
      #endif
    }
  #else

@Epixu
Copy link
Contributor Author

Epixu commented Aug 12, 2024

I believe it is ready for merge @mr-c

@mr-c mr-c enabled auto-merge (rebase) August 12, 2024 12:30
@mr-c
Copy link
Collaborator

mr-c commented Aug 12, 2024

Thank you @Epixu ! I squashed your commit and made some other small changes

@mr-c mr-c merged commit f47e3c5 into simd-everywhere:master Aug 12, 2024
91 of 92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants