Jit: Threads: Add Fence, Wait, and Notify #285

matetokodi · 2024-09-02T12:03:41Z

No description provided.

src/jit/ByteCodeParser.cpp

zherczeg · 2024-09-03T08:08:38Z

src/jit/MemoryUtilInl.h

+static void atomicNotifyCallback(ExecutionContext* context, sljit_s32 memOffset)
+{
+    Instance* instance = context->instance;
+    Memory** memories = reinterpret_cast<Memory**>(reinterpret_cast<uintptr_t>(instance) + Instance::alignedSize());


I suspect we can get this with a helper function.

src/jit/MemoryUtilInl.h

zherczeg · 2024-09-03T08:11:02Z

src/jit/MemoryUtilInl.h

+    Instance* instance = context->instance;
+    Memory** memories = reinterpret_cast<Memory**>(reinterpret_cast<uintptr_t>(instance) + Instance::alignedSize());
+    if (!memories[0]->isShared()) {
+        context->tmp1[0] = 1;


Is this an error code? What about returning with an error code from this function.

zherczeg · 2024-09-03T08:12:40Z

src/jit/MemoryUtilInl.h

+        sljit_s32 offset = atomicWait32Operation->offset();
+
+        Operand* operands = instr->operands();
+        MemAddress addr(MemAddress::CheckNaturalAlignment, instr->requiredReg(0), instr->requiredReg(1), instr->requiredReg(2));


Oh this uses MemAddress, then it needs to be in the MemoryInl.h

src/jit/MemoryUtilInl.h

zherczeg · 2024-09-03T08:14:51Z

src/jit/MemoryUtilInl.h

+
+        sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1), count.arg, count.argw);
+
+        sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, memOffset.arg, memOffset.argw);


It seems the address is already computed, can we used it in any way? The offset is not passed in any way.

The callbacks use offsets and not the absolute address, so I do not think so.

zherczeg · 2024-09-04T09:13:02Z

src/jit/MemoryInl.h

+static sljit_s32 atomicWaitCallback(ExecutionContext* context, sljit_s32 memOffset, sljit_s32 size)
+{
+    Instance* instance = context->instance;
+    if (!instance->memory(0)->isShared()) {


You should add some newlines to this function.

zherczeg · 2024-09-04T09:14:18Z

src/jit/MemoryInl.h

+    CompileContext* context = CompileContext::get(compiler);
+    sljit_s32 size = (instr->opcode() == ByteCode::MemoryAtomicWait64Opcode ? 8 : 4);
+
+    MemoryAtomicWait32* atomicWait32Operation = reinterpret_cast<MemoryAtomicWait32*>(instr->byteCode());


Is this type casting is valid for AtomicWait64?

Still valid.

This looks somewhat confusing, because MemoryAtomicWait32 and MemoryAtomicWait64 are handled together here.
Would it be better to merge these two bytecodes into one unfied bytecode like MemoryAtomicWait?

zherczeg · 2024-09-04T09:18:03Z

src/jit/MemoryInl.h

+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp2), timeout.arg, timeout.argw);
+#endif /* SLJIT_32BIT_ARCHITECTURE */
+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, memOffset.arg, memOffset.argw);


Should not this be SLJIT_MOV32? Furthermore, how the constant offset is passed?

zherczeg · 2024-09-04T09:19:03Z

src/jit/MemoryInl.h

+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1), count.arg, count.argw);
+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, memOffset.arg, memOffset.argw);


Same issue with offset

zherczeg

The patch should be ready for review

zherczeg · 2024-09-10T08:05:46Z

.github/workflows/actions.yml

@@ -271,6 +271,7 @@ jobs:
    - name: Run Tests
      run: |
        $RUNNER --engine="$GITHUB_WORKSPACE/out/extended/walrus" wasm-test-extended
+        $RUNNER --jit --engine="$GITHUB_WORKSPACE/out/extended/walrus" wasm-test-extended


After this patch, does the extension director work with jit?

What do you mean by extension director?

I mean all tests will pass with jit

Ah, yes they do.

zherczeg · 2024-09-10T08:06:29Z

src/jit/ByteCodeParser.cpp

@@ -1958,6 +1961,51 @@ static void compileFunction(JITCompiler* compiler)
            operands[3] = STACK_OFFSET(atomicRmwCmpxchg->dstOffset());
            break;
        }
+        case ByteCode::AtomicFenceOpcode: {
+            Instruction* instr = compiler->append(byteCode, Instruction::AtomicFence, opcode, 0, 0);


Is there no other similar opcode? Data drop maybe?

I put it together with UnreachableOpcode, which is also a 0-0 argument non-callback instruction (Data Drop is marked as a callback)

zherczeg · 2024-09-10T08:10:06Z

src/jit/MemoryInl.h

+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp2), timeout.arg, timeout.argw);
+#endif /* SLJIT_32BIT_ARCHITECTURE */
+
+    sljit_emit_op1(compiler, SLJIT_MOV32, SLJIT_R1, 0, memOffset.arg, memOffset.argw);


Waht about sljit_s32 offset = atomicWait32Operation->offset(); immediate?

I realized I forgot to use the modified wait callback that uses the absolute address; so I no longer need the offset, and use addr.basereg instead, like it was already used in notify.

zherczeg · 2024-09-10T08:10:19Z

src/jit/MemoryInl.h

+    CompileContext* context = CompileContext::get(compiler);
+    sljit_s32 size = (instr->opcode() == ByteCode::MemoryAtomicWait64Opcode ? 8 : 4);
+
+    MemoryAtomicWait32* atomicWait32Operation = reinterpret_cast<MemoryAtomicWait32*>(instr->byteCode());


Still valid.

zherczeg · 2024-09-10T08:11:34Z

test/extended/threads/atomic.wast

@@ -4,6 +4,7 @@
  (memory 1 1 shared)

  (func (export "init") (param $value i64) (i64.store (i32.const 0) (local.get $value)))
+  (func (export "initOffset") (param $value i64) (param $offset i32) (i64.store (local.get $offset) (local.get $value)))


I think these files comes from the standard. We should not add custom modifications here. They should go separate files.

zherczeg · 2024-09-11T07:09:45Z

test/extended/threads/atomic_with_offsets.wast

+
+  (func (export "initOffset") (param $value i64) (param $offset i32) (i64.store (local.get $offset) (local.get $value)))
+
+  (func (export "i32.atomic.load") (param $addr i32) (result i32) (i32.atomic.load (local.get $addr)))


Where is the offset? It should look like:

i32.atomic.load offset=32 (local.get $addr))

The offset does not need to be aligned, just the final address.

zherczeg · 2024-09-11T07:50:10Z

test/extended/threads/atomic_with_offsets.wast

+(module
+  (memory 1 1 shared)
+
+  (func (export "initOffset") (param $value i64) (param $offset i32) (i64.store offset=33 (local.get $offset) (local.get $value)))


Much better, but could we add some variations to the offset not just 33? Some aligned, some unaligned, and some bigger values.

zherczeg · 2024-09-11T08:06:52Z

src/jit/MemoryInl.h

+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp2), timeout.arg, timeout.argw);
+#endif /* SLJIT_32BIT_ARCHITECTURE */
+
+    sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, addr.baseReg, 0);


Are you sure this is correct?

I am pretty sure it is if used with the MemAddress::CheckNaturalAlignment | MemAddress::AbsoluteAddress memory address flags; or should we use SLJIT_EXTRACT_REG(addr.memArg.arg) like it is in emitAtomic?

The proper way is the latter.

zherczeg

LGTM

clover2123 · 2024-09-12T12:09:38Z

src/jit/ByteCodeParser.cpp

+            Instruction* instr = compiler->append(byteCode, Instruction::AtomicNotify, opcode, 2, 1);
+            instr->addInfo(Instruction::kIsCallback);
+
+            MemoryAtomicNotify* memoryAtomicWait = reinterpret_cast<MemoryAtomicNotify*>(byteCode);


This seems a typo (memoryAtomicWait -> memoryAtomicNotify)

clover2123 · 2024-09-12T12:17:14Z

src/jit/MemoryInl.h

+    CompileContext* context = CompileContext::get(compiler);
+    sljit_s32 size = (instr->opcode() == ByteCode::MemoryAtomicWait64Opcode ? 8 : 4);
+
+    MemoryAtomicWait32* atomicWait32Operation = reinterpret_cast<MemoryAtomicWait32*>(instr->byteCode());


This looks somewhat confusing, because MemoryAtomicWait32 and MemoryAtomicWait64 are handled together here.
Would it be better to merge these two bytecodes into one unfied bytecode like MemoryAtomicWait?

zherczeg · 2024-09-16T07:23:44Z

src/interpreter/ByteCode.h

@@ -827,6 +828,33 @@ class Const128 : public ByteCode {
    uint32_t m_value[4];
 };

+// dummy ByteCode for 3-input 1-output operation with offset
+class TernaryOperationOffset : public ByteCode {


Based on the other common bases, the name should be ByteCodeOffset4Value. The class should be declared after ByteCodeOffset2Value.

Add a few test cases with non-zero offsets Signed-off-by: Máté Tokodi [email protected]

zherczeg · 2024-09-21T06:38:50Z

@clover2123 could you review this patch?

clover2123

LGTM

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 6baa21d to b33af1f Compare September 2, 2024 15:02

zherczeg requested changes Sep 3, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch from b33af1f to e8b6f93 Compare September 3, 2024 12:14

zherczeg requested changes Sep 4, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 8f946f1 to 668df67 Compare September 9, 2024 15:09

zherczeg requested changes Sep 10, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 1bced88 to 5bafd6d Compare September 10, 2024 11:47

matetokodi marked this pull request as ready for review September 10, 2024 12:37

matetokodi requested review from ksh8281 and clover2123 as code owners September 10, 2024 12:37

zherczeg reviewed Sep 11, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch from 5bafd6d to 76a4efa Compare September 11, 2024 07:41

zherczeg reviewed Sep 11, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 831011f to ae0618a Compare September 11, 2024 12:04

zherczeg approved these changes Sep 11, 2024

View reviewed changes

zherczeg mentioned this pull request Sep 11, 2024

Reduce stack usage of jit #287

Merged

clover2123 reviewed Sep 12, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 012326d to 815c9b5 Compare September 13, 2024 15:19

zherczeg reviewed Sep 16, 2024

View reviewed changes

matetokodi force-pushed the jit_threads_fence_wait_notify branch 2 times, most recently from 161c745 to 430f844 Compare September 16, 2024 09:48

Jit: Threads: Add Fence, Wait, and Notify

f448e11

Add a few test cases with non-zero offsets Signed-off-by: Máté Tokodi [email protected]

matetokodi force-pushed the jit_threads_fence_wait_notify branch from 430f844 to f448e11 Compare September 16, 2024 10:00

clover2123 approved these changes Sep 23, 2024

View reviewed changes

clover2123 merged commit e5e2c64 into Samsung:main Sep 23, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jit: Threads: Add Fence, Wait, and Notify #285

Jit: Threads: Add Fence, Wait, and Notify #285

matetokodi commented Sep 2, 2024

zherczeg Sep 3, 2024

zherczeg Sep 3, 2024

zherczeg Sep 3, 2024

zherczeg Sep 3, 2024

matetokodi Sep 3, 2024

zherczeg Sep 4, 2024

zherczeg Sep 4, 2024

zherczeg Sep 10, 2024

clover2123 Sep 12, 2024

zherczeg Sep 4, 2024

zherczeg Sep 4, 2024

zherczeg left a comment

zherczeg Sep 10, 2024

matetokodi Sep 10, 2024

zherczeg Sep 10, 2024

matetokodi Sep 10, 2024

zherczeg Sep 10, 2024

matetokodi Sep 10, 2024

zherczeg Sep 10, 2024

matetokodi Sep 10, 2024

zherczeg Sep 10, 2024

zherczeg Sep 10, 2024

zherczeg Sep 11, 2024

zherczeg Sep 11, 2024

zherczeg Sep 11, 2024

matetokodi Sep 11, 2024

zherczeg Sep 11, 2024

zherczeg left a comment

clover2123 Sep 12, 2024

clover2123 Sep 12, 2024

zherczeg Sep 16, 2024

zherczeg commented Sep 21, 2024

clover2123 left a comment


		sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_MEM1(kContextReg), OffsetOfContextField(tmp1), count.arg, count.argw);

		sljit_emit_op1(compiler, SLJIT_MOV, SLJIT_R1, 0, memOffset.arg, memOffset.argw);


		(func (export "initOffset") (param $value i64) (param $offset i32) (i64.store (local.get $offset) (local.get $value)))

		(func (export "i32.atomic.load") (param $addr i32) (result i32) (i32.atomic.load (local.get $addr)))

Jit: Threads: Add Fence, Wait, and Notify #285

Jit: Threads: Add Fence, Wait, and Notify #285

Conversation

matetokodi commented Sep 2, 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

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

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

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

Choose a reason for hiding this comment

zherczeg commented Sep 21, 2024

clover2123 left a comment

Choose a reason for hiding this comment