Implement KT-73565 and basic box tests for memcpy, memmove, memset and memcmp intrinsics #5385

KitsuneAlex · 2024-12-07T07:40:25Z

As described in the associated YouTrack issue, this PR aims to add memory intrinsics for Kotlin/Native for functions such as memset, memcpy, memmove and memcmp (throught libc). This allows optimizing a lot of per-element loop based functions with much faster optimized code.

I also implemented a public API for the intrinsics, supporting both untyped NativePointed addresses, and type safe extension functions for CPointer.

Very basic tests have been implemented in kt73565.kt for every function to make sure they work.

If you have any comments or nitpicks, feel free to let me know.
Cheers.

KitsuneAlex · 2024-12-07T16:56:58Z

Adding some changes to this, i wasn't happy with the names of the extension functions for CPointer and i also forgot to add the TARGET header for the box test.

KitsuneAlex · 2024-12-09T11:27:46Z

...tive/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/CodeGenerator.kt

+import kotlin.collections.set
+import kotlin.collections.sortedBy
+import kotlin.collections.toList
+import kotlin.collections.toTypedArray


Don't know why my formatter did this with the project settings.. can fix if desired.

Yes, please.

sbogolepov

Thanks! Great work overall, but it requires a few minor adjustments :)

sbogolepov · 2024-12-12T08:08:07Z

kotlin-native/Interop/Runtime/src/jvm/kotlin/kotlinx/cinterop/JvmNativeMem.kt

@@ -90,6 +98,65 @@ internal object nativeMemUtils {
    fun copyMemory(dest: NativePointed, length: Int, src: NativePointed) =
            unsafe.copyMemory(src.address, dest.address, length.toLong())

+    fun memset(mem: NativePointed, value: Byte, size: Int) {


There is no need to add intrinsics to the JVM interop as they are unused there.

Huh, it didn't compile without them for me in my personal fork, that's odd. Will remove ^^

Yeah i have to add them there too it seems, at least if i want to add this to nativeMemUtils, the code does not compile when the functions are not defined in both files. Everything breaks :/

sbogolepov · 2024-12-12T08:09:53Z

...tive/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/CodeGenerator.kt

+import kotlin.collections.set
+import kotlin.collections.sortedBy
+import kotlin.collections.toList
+import kotlin.collections.toTypedArray


Yes, please.

sbogolepov · 2024-12-12T08:11:44Z

...tive/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/llvm/CodeGenerator.kt

@@ -391,8 +419,14 @@ internal class StackLocalsManagerImpl(
        val bbInitStackLocals: LLVMBasicBlockRef
 ) : StackLocalsManager {
    private var scopeDepth = 0
-    override fun enterScope() { scopeDepth++ }
-    override fun exitScope() { scopeDepth-- }
+    override fun enterScope() {


A nitpick: it is usually a good idea to separate formatting changes from actual ones. Beyond other things, it makes review easier :)

Fair enough, will keep that in mind. I'm just way too used to hitting CTRL+ALT+L after everytime i finish writing something somewhere hahaha.

sbogolepov · 2024-12-12T08:57:57Z

kotlin-native/Interop/Runtime/src/native/kotlin/kotlinx/cinterop/NativeMem.kt

-    @TypedIntrinsic(IntrinsicType.INTEROP_READ_PRIMITIVE) external fun getDouble(mem: NativePointed): Double
-    @TypedIntrinsic(IntrinsicType.INTEROP_WRITE_PRIMITIVE) external fun putDouble(mem: NativePointed, value: Double)
+    @TypedIntrinsic(IntrinsicType.INTEROP_SET_MEMORY)
+    external fun memset(mem: NativePointed, value: Byte, size: Int)


Why do we need both 32 and 64-bit versions? Are they equally portable?

From what i know about LLVM, they should be equally portable.
However, there's a reason why i split them up:

All major native platforms apart from some mobile and embedded applications are 64-bit now.
Using 64-bit everywhere for things like memcpy could waste CPU cycles for applications where 64-bits will never be used (since this affects the internal index size of the intrinsic implementation afaik), like when copying between native memory <-> kotlin arrays, only if it's probably very little.
It is still important to offer the 64-bit API to allow utilizing the full capability of the host CPU when working with unmanaged memory.

If you want, we can drop the small 32-bit optimization entirely and just cast to 64-bit size types when dealing with things like arrays etc.

I could be wrong to be honest, will look into this a little deeper.

All major native platforms apart from some mobile and embedded applications are 64-bit now.

Unfortunately, we still have 32-bit targets, and providing 64-bit API for them might be wrong.

Then we should offer both, since Kotlin does not have a size type, i think. 32-bit only is very mediocre for more serious applications imo.

sbogolepov · 2024-12-12T09:13:04Z

kotlin-native/Interop/Runtime/src/main/kotlin/kotlinx/cinterop/Utils.kt

+}
+
+@ExperimentalForeignApi
+public fun copyMemory(destMem: NativePointed, srcMem: NativePointed, size: Long) {


Do we really need both moveMemory and copyMemory functions? The second one is less safe and performance benefits might not worth it.

No real reason i added memmove apart from "might as well complete the set of three" when i was reading the LLVM manual to look up the copy intrinsic. Can remove if you want :)

Yep, I tend to think that memmove should be sufficient for now.

Hm, not entirely happy with that to be honest, since 99% of the time you'd use memcpy, not memmove but that works.

sbogolepov · 2024-12-12T09:21:45Z

kotlin-native/Interop/Runtime/src/main/kotlin/kotlinx/cinterop/Utils.kt

+}
+
+@ExperimentalForeignApi
+public inline fun <reified T : CVariable> CPointer<T>.compareBlock(dest: CPointer<T>, length: Int): Int {


Maybe compareContentsWith?

I like that, will change.

sbogolepov · 2024-12-12T09:24:11Z

kotlin-native/Interop/Runtime/src/main/kotlin/kotlinx/cinterop/Utils.kt

+}
+
+@ExperimentalForeignApi
+public inline fun <reified T : CVariable> CPointer<T>.setBlock(value: Byte, length: Int) {


Maybe just fillWith?

Also a good name, will change.

sbogolepov · 2024-12-12T09:27:11Z

kotlin-native/Interop/Runtime/src/main/kotlin/kotlinx/cinterop/Utils.kt

@@ -727,3 +735,43 @@ public fun COpaquePointer.readBytes(count: Int): ByteArray {
    nativeMemUtils.getByteArray(this.reinterpret<ByteVar>().pointed, result, count)
    return result
 }
+
+@ExperimentalForeignApi
+public fun setMemory(mem: NativePointed, value: Byte, size: Long) {


Could you please add a small KDoc for all new public functions?

sbogolepov · 2024-12-12T09:31:34Z

native/native.tests/testData/codegen/cinterop/kt73565.kt

+    data1.usePinned { pinned ->
+        pinned.addressOf(0).setBlock(0, data1.size)
+    }
+    if (data1.contentEquals(data2)) {


Checking the new value directly would be more robust.

True, will fix.

sbogolepov · 2024-12-12T09:32:06Z

native/native.tests/testData/codegen/cinterop/kt73565.kt

+import kotlin.test.*
+import kotlinx.cinterop.*
+
+fun box(): String {


Please add tests for top-level functions as well.

Good point, don't know why i didn't think of that, will add.

KitsuneAlex · 2024-12-16T07:27:33Z

Will update this in the following days, was a little busy this weekend :)

Implement KT-73565 and basic box tests

04ff86f

Rename CPointer extension, add missing target header from box test

1c634a7

KitsuneAlex commented Dec 9, 2024

View reviewed changes

sbogolepov self-requested a review December 12, 2024 09:15

sbogolepov requested changes Dec 12, 2024

View reviewed changes

projedi self-requested a review December 12, 2024 10:14

SvyatoslavScherbina added the Native label Dec 12, 2024

Merge branch 'master' into KT-73565

b2c8a21

KitsuneAlex added 3 commits December 19, 2024 13:31

Merge branch 'master' into KT-73565

07eb307

Merge branch 'master' into KT-73565

21dd4ab

Merge branch 'master' into KT-73565

ed67427

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement KT-73565 and basic box tests for memcpy, memmove, memset and memcmp intrinsics #5385

Implement KT-73565 and basic box tests for memcpy, memmove, memset and memcmp intrinsics #5385

KitsuneAlex commented Dec 7, 2024

KitsuneAlex commented Dec 7, 2024

KitsuneAlex Dec 9, 2024

sbogolepov Dec 12, 2024

sbogolepov left a comment

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

KitsuneAlex Dec 20, 2024

sbogolepov Dec 12, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 13, 2024

KitsuneAlex Dec 20, 2024 •

edited

Loading

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 13, 2024

KitsuneAlex Dec 20, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 12, 2024 •

edited

Loading

KitsuneAlex Dec 12, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

sbogolepov Dec 12, 2024

KitsuneAlex Dec 12, 2024

KitsuneAlex commented Dec 16, 2024

Implement KT-73565 and basic box tests for memcpy, memmove, memset and memcmp intrinsics #5385

Are you sure you want to change the base?

Implement KT-73565 and basic box tests for memcpy, memmove, memset and memcmp intrinsics #5385

Conversation

KitsuneAlex commented Dec 7, 2024

KitsuneAlex commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbogolepov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitsuneAlex Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbogolepov Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitsuneAlex commented Dec 16, 2024

KitsuneAlex Dec 20, 2024 •

edited

Loading

sbogolepov Dec 12, 2024 •

edited

Loading