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

[Windows/ARM64] Fix raycast/embree ARM64 build with LLVM/MinGW. #93364

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jun 19, 2024

No description provided.

@bruvzg bruvzg added this to the 4.x milestone Jun 19, 2024
@bruvzg bruvzg requested a review from a team as a code owner June 19, 2024 18:22
@bruvzg bruvzg force-pushed the mingw-llvm-arm64 branch from 1bfe179 to f48d6f9 Compare June 20, 2024 12:09
@bruvzg bruvzg requested a review from a team as a code owner June 20, 2024 12:09
@@ -102,7 +102,9 @@
#include <stdint.h>
#include <stdlib.h>

#if defined(_WIN32)
// -- GODOT start --
#if defined(_WIN32) && !defined(__clang__)
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't it work on llvm-mingw specifically but works on mingw-w64-gcc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely it's not clang but UCRT specific, but _mm_malloc is not found with this define.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is ARM specific code, so MinGW-GCC might have it missing as well.

Copy link
Member

@akien-mga akien-mga Jun 20, 2024

Choose a reason for hiding this comment

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

Should we use !defined(__MINGW32__) then? It would make the comment lie though.

Copy link
Member

Choose a reason for hiding this comment

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

Either way, worth PR'ing upstream to get more eyes on it and find out the proper fix there: https://github.com/DLTcollab/sse2neon

Downstream I'm fine with either option to get this to compile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have checked headers for both MinGW/GCC and MinGW/LLVM, and it's only defined for x86 in both, so !defined(__MINGW32__) should be more correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

MSVC define it for all architectures, and it's just a macro for the same _aligned_malloc I have added, so it's the same.

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe it should just be _MSC_VER?
Is it defined for clang-cl?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea, but __MINGW32__ definitely should not be defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Upstream PR - DLTcollab/sse2neon#637

@bruvzg bruvzg force-pushed the mingw-llvm-arm64 branch from f48d6f9 to f9b3e7b Compare June 20, 2024 12:23
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jun 20, 2024
@bruvzg bruvzg force-pushed the mingw-llvm-arm64 branch from f9b3e7b to f9b23cf Compare June 20, 2024 12:40
@bruvzg bruvzg force-pushed the mingw-llvm-arm64 branch from f9b23cf to 04d70c1 Compare June 20, 2024 12:52
@akien-mga akien-mga merged commit 6fec188 into godotengine:master Jun 20, 2024
15 of 16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants