-
Notifications
You must be signed in to change notification settings - Fork 15.9k
[libc] Use generic/builtin.h for Emscripten in memory utils #177474
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
Conversation
|
@llvm/pr-subscribers-libc Author: None (google-yfyang) ChangesWASM supports bulk memory operations and can benefit from using generic/builtin.h. Full diff: https://github.com/llvm/llvm-project/pull/177474.diff 3 Files Affected:
diff --git a/libc/src/string/memory_utils/inline_memcpy.h b/libc/src/string/memory_utils/inline_memcpy.h
index 13975e6b3bd0e..931d2821be3a9 100644
--- a/libc/src/string/memory_utils/inline_memcpy.h
+++ b/libc/src/string/memory_utils/inline_memcpy.h
@@ -31,7 +31,7 @@
#elif defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
#include "src/string/memory_utils/riscv/inline_memcpy.h"
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMCPY inline_memcpy_riscv
-#elif defined(LIBC_TARGET_ARCH_IS_GPU)
+#elif defined(LIBC_TARGET_ARCH_IS_GPU) || defined(LIBC_TARGET_ARCH_IS_WASM)
#include "src/string/memory_utils/generic/builtin.h"
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMCPY inline_memcpy_builtin
#else
diff --git a/libc/src/string/memory_utils/inline_memmove.h b/libc/src/string/memory_utils/inline_memmove.h
index 71a28c32e4c56..31627669b06bf 100644
--- a/libc/src/string/memory_utils/inline_memmove.h
+++ b/libc/src/string/memory_utils/inline_memmove.h
@@ -29,7 +29,7 @@
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMMOVE_SMALL_SIZE \
inline_memmove_no_small_size
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMMOVE_FOLLOW_UP inline_memmove_riscv
-#elif defined(LIBC_TARGET_ARCH_IS_GPU)
+#elif defined(LIBC_TARGET_ARCH_IS_GPU) || defined(LIBC_TARGET_ARCH_IS_WASM)
#include "src/string/memory_utils/generic/builtin.h"
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMMOVE_SMALL_SIZE \
inline_memmove_no_small_size
diff --git a/libc/src/string/memory_utils/inline_memset.h b/libc/src/string/memory_utils/inline_memset.h
index e41bdb626d60e..da85f09d0db57 100644
--- a/libc/src/string/memory_utils/inline_memset.h
+++ b/libc/src/string/memory_utils/inline_memset.h
@@ -27,7 +27,7 @@
#elif defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
#include "src/string/memory_utils/riscv/inline_memset.h"
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMSET inline_memset_riscv
-#elif defined(LIBC_TARGET_ARCH_IS_GPU)
+#elif defined(LIBC_TARGET_ARCH_IS_GPU) || defined(LIBC_TARGET_ARCH_IS_WASM)
#include "src/string/memory_utils/generic/builtin.h"
#define LIBC_SRC_STRING_MEMORY_UTILS_MEMSET inline_memset_builtin
#else
|
|
Diff between emscripten-core/emscripten#26151 and LLVM 21.1.8 has this too: diff --git a/libc/src/__support/macros/properties/architectures.h b/libc/src/__support/macros/properties/architectures.h
index c88956ff4114..817ced4e95c2 100644
--- a/libc/src/__support/macros/properties/architectures.h
+++ b/libc/src/__support/macros/properties/architectures.h
@@ -41,6 +41,10 @@
#define LIBC_TARGET_ARCH_IS_ARM
#endif
+#if defined(__wasm__)
+#define LIBC_TARGET_ARCH_IS_WASM
+#endif
+
#if defined(__aarch64__) || defined(__arm64__) || defined(_M_ARM64)
#define LIBC_TARGET_ARCH_IS_AARCH64
#endifShould we include this here too? |
This already went into the HEAD 4 months ago. https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/properties/architectures.h. Presumably LLVM 21.1.8 isn't new enough to see this change reflected. |
|
@michaelrj-google Could you please take a look? |
jhuber6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you've verified that __builtin_memmove and similar do not emit libcalls in their implementation? Should be fine.
This makes llvm libc on par with libcxx and libcxxabi. #26058 The remaining local changes to libc sources are very minor and is all in llvm/llvm-project#177474. Once both of these PRs go in, we can try syncing it to the LLVM upstream HEAD.
michaelrj-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as long as you've confirmed that the builtins don't call the library functions (as jhuber suggested)
|
Yes, https://godbolt.org/z/T69z9TYc8. Looks like no libcalls are emitted. |
WASM supports bulk memory operations and can benefit from using generic/builtin.h.