Jit: Threads: Add RmwCmpxchg #282

matetokodi · 2024-08-07T10:32:13Z

Add RmwCmpxchg

Depends on #280

zherczeg · 2024-08-14T05:21:35Z

Please update the patch, so I can review the actual changes.

zherczeg · 2024-08-14T05:26:19Z

src/jit/MemoryInl.h

+    sljit_s32 type = SLJIT_ARGS3V(P, P, P);
+    sljit_s32 faddr GET_FUNC_ADDR(sljit_sw, atomicRmwCmpxchg64);
+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R0, 0, SLJIT_EXTRACT_REG(addr.memArg.arg), 0);


The problem here is that if R0 is overwritten and used by srcExpectedArgPair or srcValueArgPair, then an incorrect value is written. I would copy the arguments to memory, if needed, and then initialize R0 to R2 after that.

src/jit/MemoryInl.h

zherczeg · 2024-08-21T06:44:33Z

src/jit/MemoryInl.h

+        sljit_emit_op2(compiler, SLJIT_ADD, SLJIT_R2, 0, kFrameReg, 0, SLJIT_IMM, srcValueArgPair.arg1w - WORD_LOW_OFFSET);
+    }
+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R0, 0, SLJIT_EXTRACT_REG(addr.memArg.arg), 0);


If addr.memArg.arg is SLJIT_R1 or SLJIT_R2, the value has been overwritten. This should happen before the first add operation, and this should also be checked: addr.memArg.arg != SLJIT_R0

zherczeg · 2024-08-21T06:47:35Z

src/jit/MemoryInl.h

@@ -1147,9 +1214,14 @@ static void emitAtomic(sljit_compiler* compiler, Instruction* instr)
    case ByteCode::I64AtomicRmwAndOpcode:
    case ByteCode::I64AtomicRmwOrOpcode:
    case ByteCode::I64AtomicRmwXorOpcode:
-    case ByteCode::I64AtomicRmwXchgOpcode: {
+    case ByteCode::I64AtomicRmwXchgOpcode:
+    case ByteCode::I64AtomicRmwCmpxchgOpcode: {


If this is the first case, the if operation below is unnecessary.

zherczeg · 2024-08-21T06:48:54Z

src/jit/MemoryInl.h

-    AtomicRmw* rmwOperation = reinterpret_cast<AtomicRmw*>(instr->byteCode());
-    offset = rmwOperation->offset();
+    if (operation != OP_CMPXCHG) {
+        AtomicRmw* rmwOperation = reinterpret_cast<AtomicRmw*>(instr->byteCode());


This is just the copy of the old code, isn't it?

It is; the previous cases are int this early return block if (operation != OP_CMPXCHG), which is followed by the code for Cmpxchg

zherczeg · 2024-08-21T06:58:06Z

src/jit/MemoryInl.h

+
+    compareFalse = sljit_emit_cmp(compiler, SLJIT_NOT_EQUAL, SLJIT_TMP_DEST_REG, 0, srcExpectedReg, 0);
+    if (!(operationSize & SLJIT_32) && operationSize != SLJIT_MOV32) {
+        compareTopFalse = sljit_emit_cmp(compiler, SLJIT_NOT_EQUAL, SLJIT_IMM, 0, tmpReg, 0);


I don't understand something. You check that the upper 32 bit must be zero, but don't check the bits of the lower 32 bit for 8/16 bit operations. Furthermore this should happen before sljit_emit_atomic_load since srcExpectedPair.arg2 should not change. Then tmpReg is changed to srcValue.

The lower part does not need to be checked for zeroes, because when loading a smaller value, the rest of the register is zeroed out.

It also cannot happen before the load, because the loaded value is required for the result; It would be possible to not the atomic load if the upper half is not zero, but a load would still be required for the result later, and it would complicate the code, I find this way cleaner.

zherczeg · 2024-08-26T07:22:17Z

src/jit/MemoryInl.h

+    if (srcExpectedArgPair.arg1 != SLJIT_MEM1(kFrameReg)) {
+        sljit_emit_op1(compiler, SLJIT_MOV, dstArgPair.arg1, dstArgPair.arg1w, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1) + WORD_LOW_OFFSET);
+        sljit_emit_op1(compiler, SLJIT_MOV, dstArgPair.arg2, dstArgPair.arg2w, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1) + WORD_HIGH_OFFSET);
+    } else {


Something is not right here. Is the expected value overwritten by the result? This should not happen, since the expected value is a read-only data.

Probably we should copy expected value to destination first (if needed), then do the update using the destination, and do nothing if destnation is a memory location.

yes, std::atomic<T>::compare_exchange_weak modifies the expected value to the value loaded from memory if the comparison fails

I have adjusted it so we use dst for the expected value parameter if it is memory, and use context tmp if it is not.

zherczeg · 2024-08-26T07:25:30Z

src/jit/MemoryInl.h

-    if (operation != OP_XCHG) {
-        sljit_emit_op2(compiler, operation, srcReg, 0, SLJIT_TMP_DEST_REG, 0, srcReg, 0);
+
+    if (!(operationSize & SLJIT_32) && operationSize != SLJIT_MOV32) {


This should be done before sljit_emit_atomic_load

zherczeg · 2024-08-26T07:27:01Z

src/jit/MemoryInl.h

-    sljit_emit_atomic_store(compiler, operationSize | SLJIT_SET_ATOMIC_STORED, srcReg, SLJIT_EXTRACT_REG(addr.memArg.arg), SLJIT_TMP_DEST_REG);
+
+    compareFalse = sljit_emit_cmp(compiler, SLJIT_NOT_EQUAL, SLJIT_TMP_DEST_REG, 0, srcExpectedReg, 0);
+    sljit_emit_op1(compiler, SLJIT_MOV, tmpReg, 0, srcValue.arg, srcValue.argw);


This should be done before sljit_emit_atomic_load as well (this is correct in the non SLJIT_32BIT_ARCHITECTURE case)

zherczeg · 2024-08-26T12:56:00Z

src/jit/MemoryInl.h

+    } else if (srcExpectedArgPair.arg1 != SLJIT_MEM1(kFrameReg)) {
+        sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1) + WORD_LOW_OFFSET, srcExpectedArgPair.arg1, srcExpectedArgPair.arg1w);
+        sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1) + WORD_HIGH_OFFSET, srcExpectedArgPair.arg2, srcExpectedArgPair.arg2w);
+    }


What happens if both if-s failed?

This should look like this:

if dst is SLJIT_MEM1(kFrameReg) if dst != expected arg move expected to dst else move expected to context field tmp1

zherczeg

Close to ready. Please change draft mode to to ready.

zherczeg · 2024-08-27T09:41:17Z

src/jit/MemoryInl.h

+    sljit_s32 faddr GET_FUNC_ADDR(sljit_sw, atomicRmwCmpxchg64);
+
+    if (srcExpectedArgPair.arg1 == SLJIT_MEM1(kFrameReg)) {
+        if (dstArgPair.arg1 != srcExpectedArgPair.arg1 || dstArgPair.arg1w != srcExpectedArgPair.arg1w) {


Since these cannot partly overlap, one if is enough.

zherczeg · 2024-08-27T09:46:48Z

src/jit/MemoryInl.h

+    if (!(operationSize & SLJIT_32) && operationSize != SLJIT_MOV32) {
+        compareTopFalse = sljit_emit_cmp(compiler, SLJIT_NOT_EQUAL, SLJIT_IMM, 0, srcExpectedPair.arg2, srcExpectedPair.arg2w);
+    }
+    sljit_emit_op1(compiler, SLJIT_MOV, tmpReg, 0, srcValue.arg, srcValue.argw);


This is just a note. It looks like webassembly is sensitive to the expected value upper bits (must be 0), but does not care about the srcValue upper bits. Does not make much sense.

Signed-off-by: Máté Tokodi [email protected]

zherczeg

LGTM

zherczeg reviewed Aug 14, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_rmw_cmpxchg branch 2 times, most recently from 982e877 to c7c1d23 Compare August 21, 2024 06:10

zherczeg requested changes Aug 21, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_rmw_cmpxchg branch from c7c1d23 to 48e1d7d Compare August 22, 2024 14:35

zherczeg requested changes Aug 26, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_rmw_cmpxchg branch from 48e1d7d to 6e9afc8 Compare August 26, 2024 12:44

zherczeg reviewed Aug 26, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_rmw_cmpxchg branch from 6e9afc8 to 812262f Compare August 26, 2024 14:23

zherczeg requested changes Aug 27, 2024

View reviewed changes

Jit: Threads: Add RmwCmpxchg

cd4aed4

Signed-off-by: Máté Tokodi [email protected]

matetokodi force-pushed the jit_threads_rmw_cmpxchg branch from 812262f to cd4aed4 Compare August 27, 2024 11:02

matetokodi marked this pull request as ready for review August 27, 2024 11:24

matetokodi requested review from ksh8281 and clover2123 as code owners August 27, 2024 11:24

zherczeg approved these changes Aug 28, 2024

View reviewed changes

clover2123 approved these changes Aug 30, 2024

View reviewed changes

clover2123 merged commit 238685b into Samsung:main Aug 30, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jit: Threads: Add RmwCmpxchg #282

Jit: Threads: Add RmwCmpxchg #282

matetokodi commented Aug 7, 2024

zherczeg commented Aug 14, 2024

zherczeg Aug 14, 2024

zherczeg Aug 21, 2024

zherczeg Aug 21, 2024

zherczeg Aug 21, 2024

matetokodi Aug 22, 2024

zherczeg Aug 21, 2024

matetokodi Aug 22, 2024

zherczeg Aug 26, 2024

matetokodi Aug 26, 2024

zherczeg Aug 26, 2024

zherczeg Aug 26, 2024

zherczeg Aug 26, 2024

zherczeg left a comment

zherczeg Aug 27, 2024

zherczeg Aug 27, 2024

zherczeg left a comment

Jit: Threads: Add RmwCmpxchg #282

Jit: Threads: Add RmwCmpxchg #282

Conversation

matetokodi commented Aug 7, 2024

zherczeg commented Aug 14, 2024

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

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment