-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc] implement memalignment
#132493
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
[libc] implement memalignment
#132493
Conversation
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-libc Author: Mohamed Emad (hulxv) ChangesThis patch adds the Closes #132300 Full diff: https://github.com/llvm/llvm-project/pull/132493.diff 2 Files Affected:
diff --git a/libc/src/stdlib/memalignment.c b/libc/src/stdlib/memalignment.c
new file mode 100644
index 0000000000000..bf182f16ea138
--- /dev/null
+++ b/libc/src/stdlib/memalignment.c
@@ -0,0 +1,19 @@
+#include "src/stdlib/memalignment.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(size_t, memalignment, (const void *p)) {
+ if (p == NULL) {
+ return 0;
+ }
+
+ uintptr_t addr = (uintptr_t)p;
+
+ // Find the rightmost set bit, which represents the maximum alignment
+ // The alignment is a power of two, so we need to find the largest
+ // power of two that divides the address
+ return addr & (~addr + 1);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/memalignment.h b/libc/src/stdlib/memalignment.h
new file mode 100644
index 0000000000000..8ed900122a153
--- /dev/null
+++ b/libc/src/stdlib/memalignment.h
@@ -0,0 +1,22 @@
+//===-- Implementation header for memalignment --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDLIB_MEM_ALIGNMENT_H
+#define LLVM_LIBC_SRC_STDLIB_MEM_ALIGNMENT_H
+
+#include "src/__support/macros/config.h"
+#include <stddef.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+size_t memalignment(const void* p);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STDLIB_LDIV_H
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@michaelrj-google Should I add it to |
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.
Unrelated to this patch: I think this is also one of the freestanding functions, I wonder what the plan is to provide that? Normally those go in the clang resource directory headers but we don't have a freestanding stdlib.h there yet.
libc/src/stdlib/memalignment.c
Outdated
| // Find the rightmost set bit, which represents the maximum alignment | ||
| // The alignment is a power of two, so we need to find the largest | ||
| // power of two that divides the address | ||
| return addr & (~addr + 1); |
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.
| return addr & (~addr + 1); | |
| return addr & (~addr + 1); |
I can't decide if it would be better to just use ctz. They reduce to the same ASM https://godbolt.org/z/qTjxc6dPn.
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.
Made up my mind, the bit twiddling is good here, while 1ull << cpp::count_trailing_zeroes(addr) is probably easier to read, I think it performs worse for 64-bit values.
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.
then going with __builtin_ctzg is better?
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.
No, the opposite.
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.
does 1ull << cpp::count_trailing_zeroes(addr) exist? I don't find it anywhere. what about using countr_zeros?
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.
Go ahead and add this to all the entrypoints, it's a freestanding function so I'd be amazed if it didn't work on those targets. You also need to add it to the yaml if you want it to actually show up when it's installed.
ca4dbcc to
6a7d28d
Compare
6a7d28d to
edc13a9
Compare
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.
Looks like you're making good progress, just a bit more and this should be ready to land
|
@michaelrj-google all done now? |
Bot fails tests. Honestly you should probably just remove the dependencies since this doesn't need |
|
This should probably be an intrinsic so we don't introduce the ptrtoint, it's the opposite of llvm.ptrmask |
Presumably we'd want a fallback for GCC, but that definitely sounds compelling considering that this is a freestanding function, so we could just implement it as a builitin in a header for that case. |
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.
One tweak, then this is good to land.
Do you have commit access or do you need me to merge it for you?
No, I don't have access. Please merge it |
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.
Need to fix the buildbots before merging. One of the parts of the test is failing.
|
It passed the tests |
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, thanks for getting this done!
This patch adds the
memalignmentfunction to LLVM-libc, following its description in WG14 N3220, §7.24.2.1.memalignmentin/src/stdlibmemalignmentin/test/src/stdlibmemalignmenttoentrypoints.txtfor at least x86_64 and whatever you're building onmemalignmenttoinclude/stdlib.yamlCloses #132300