memutils: memmove() / memcpy() implementation#2671
memutils: memmove() / memcpy() implementation#2671baziotis wants to merge 3 commits intodlang:masterfrom baziotis:Dmemmove
Conversation
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
I recommend only including memcpy in this PR and make another one for memmove
| { // There is no overlap, use memcpy. | ||
| pragma(inline, true); | ||
| Dmemcpy(d, s, n); | ||
| }"; |
There was a problem hiding this comment.
Private. Also, what's the motivation for the split-up? Why can't the static array implementation call the other one?
There was a problem hiding this comment.
The most important reason is that I couldn't make (ref T[] dst, ref const T[] src) work when the arguments are !()(float[3], const(float[3])). I don't know why that is.
Edit: Yes, the static one can call the dynamic like this:
T[] d = dst[0 .. $];
const T[] s = src[0 .. $];
Dmemmove(d, s);
I have to check the performance penalty although my hope is that for a serious compiler, it will be none.
| } | ||
| } | ||
|
|
||
| unittest |
There was a problem hiding this comment.
This needs /// in order to be part of the documentation.
| } | ||
|
|
||
| /* Can we use SIMD? | ||
| */ |
There was a problem hiding this comment.
This comment isn't really helpful.
| }"; | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Only one line of whitespace between functions.
| */ | ||
| version (D_SIMD) | ||
| { | ||
| import core.simd: float4; |
| version (D_SIMD) | ||
| { | ||
| import core.simd: float4; | ||
| enum useSIMD = true; |
| import gcc.builtins : __builtin_ia32_storeups, __builtin_ia32_loadups; | ||
| } | ||
|
|
||
| void store16f_sse(void *dest, float4 reg) |
| } | ||
|
|
||
| /* Little SIMD library | ||
| */ |
There was a problem hiding this comment.
Maybe move into a separate module?
There was a problem hiding this comment.
That seems better to me as well. You can check point 3 in the memset PR description: #2662
I did not receive an answer and thus put them again in the same file. So, do a core/experimental/simd.d? or something else?
There was a problem hiding this comment.
one possibility is to make core.experimental.mem a package with modules cpy, move, and simd.
There was a problem hiding this comment.
Better name it memory as there's core.memory.
There was a problem hiding this comment.
Ok, I'm making a new PR. I have seen all the comments, I left them to be resolved in the new PR.
|
|
||
| /* Handle dynamic sizes < 64 with forwards move. Overlap is possible. | ||
| */ | ||
| void Dmemmove_forw_lt64(void *d, const(void) *s, size_t n) |
| __builtin_ia32_storeups(cast(float*) dest, reg); | ||
| } | ||
| } | ||
| float4 load16f_sse(const(void) *src) |
There was a problem hiding this comment.
No underscore - see the DStyle.
|
|
||
| void memmove(T)(T *dst, const T *src) | ||
| { | ||
| void *d = cast(void*) dst; |
There was a problem hiding this comment.
please write void* d not void *d, this is D not C. ditto throughout.
| mixin(arrayCode); | ||
| } | ||
|
|
||
| enum arrayCode = " |
There was a problem hiding this comment.
if you're going to use enum+mixin, please use q{} strings.
3 participants
Notes:
isArray).