Add all SIMD operations into switch, implement V128 comparison operations. #3833

jammar1 · 2024-10-01T11:08:30Z

Added all of the SIMD operations into the switch statement to avoid having to refer back and forth from the header, then implemented the V128 comparison operations using this test code:

(module
  (import "wasi_snapshot_preview1" "proc_exit" (func $proc_exit (param i32)))

  (memory (export "memory") 1)

  (func $assert_true (param v128)
    local.get 0
    v128.any_true
    i32.eqz
    if
      unreachable
    end
  )

  (func $main (export "_start")
    ;; Test v128.not
    v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
    v128.not
    v128.const i8x16 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
    i8x16.eq
    call $assert_true

    ;; Test v128.and
    v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0
    v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0
    v128.and
    v128.const i8x16 255 255 0 0 0 0 0 0 255 255 0 0 0 0 0 0
    i8x16.eq
    call $assert_true

    ;; Test v128.andnot
    v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0
    v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0
    v128.andnot
    v128.const i8x16 0 0 255 255 0 0 0 0 0 0 255 255 0 0 0 0
    i8x16.eq
    call $assert_true

    ;; Test v128.or
    v128.const i8x16 255 255 0 0 0 0 255 255 255 255 0 0 0 0 255 0
    v128.const i8x16 0 0 255 255 255 255 0 0 0 0 255 255 255 255 0 0
    v128.or
    v128.const i8x16 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 0
    i8x16.eq
    call $assert_true

    ;; Test v128.xor
    v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0
    v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0
    v128.xor
    v128.const i8x16 0 0 255 255 255 255 0 0 0 0 255 255 255 255 0 0
    i8x16.eq
    call $assert_true

    i32.const 0
    call $proc_exit
  )
)

jammar1 · 2024-10-01T11:23:23Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        addr_ret = GET_OFFSET();
+
+                        V128 result;
+                        for (i = 0; i < 16; i++) {


I think the comparison operations are fine, but this should have a proper SIMD implementation later.

perhaps worth leaving a todo so we won't forget?

I already have this one in another branch, will raise it next

loganek · 2024-10-01T18:27:37Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        V128 result;
+                        for (i = 0; i < 16; i++) {
+                            result.i8x16[i] =
+                                v1.i8x16[i] == v2.i8x16[i] ? 0xff : 0;


whereas the approach for v128.* instructions is fine, I think for opcodes like this one we should have a mechanism to override the implementation with simd instructions if they're available for a given platform. So perhaps we could have an abstraction defined and per-platform files that implement that abstraction (with the default implementation that uses plain C).

I'll do this in the next PR? I have a commit with some NEON implemented opcodes

ok sounds good to me.

loganek · 2024-10-01T18:30:11Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        break;
+                    }
+
+                    // v128 comparison operations


I think not all of them are comparison operations, but rather bitwise operations - see https://github.com/WebAssembly/spec/blob/main/proposals/simd/SIMD.md#bitwise-operations

loganek · 2024-10-01T18:31:12Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        break;
+                    }
+                    case SIMD_v128_and:
+                    {


I think there's a lot of repetition across the bitwise operations, perhaps we could define a macro for it?

wenyongh · 2024-10-05T07:38:00Z

core/iwasm/interpreter/wasm_interp_fast.c

@@ -5650,6 +5652,25 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
                GET_OPCODE();

                switch (opcode) {
+                    // Memory


Had better use /* .. */ to add comment, we usually use it for C source files. Same for below.

wenyongh

LGTM

wenyongh · 2024-10-15T03:17:26Z

core/iwasm/interpreter/wasm_interp_fast.c

+        PUT_V128_TO_ADDR(frame_lp + addr_ret, result);            \
+    } while (0)
+
+                    // In the switch statement:


Had better use /* .. */

Removed the comment

loganek

left a few minor comments, but overall LGTM

loganek · 2024-10-15T10:55:06Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        addr_ret = GET_OFFSET();
+
+                        V128 result;
+                        for (i = 0; i < 16; i++) {


perhaps worth leaving a todo so we won't forget?

loganek · 2024-10-15T10:55:16Z

core/iwasm/interpreter/wasm_interp_fast.c

+                        V128 result;
+                        for (i = 0; i < 16; i++) {
+                            result.i8x16[i] =
+                                v1.i8x16[i] == v2.i8x16[i] ? 0xff : 0;


ok sounds good to me.

loganek · 2024-10-15T10:58:15Z

core/iwasm/interpreter/wasm_interp_fast.c

@@ -487,6 +487,8 @@ wasm_interp_get_frame_ref(WASMInterpFrame *frame)

 #define POP_F64() (GET_F64_FROM_ADDR(frame_lp + GET_OFFSET()))

+#define POP_V128() (GET_V128_FROM_ADDR(frame_lp + GET_OFFSET()))


IIRC this was already merged, so you might need to rebase the branch.

Tested using ``` (module (import "wasi_snapshot_preview1" "proc_exit" (func $proc_exit (param i32))) (memory (export "memory") 1) (func $assert_true (param v128) local.get 0 v128.any_true i32.eqz if unreachable end ) (func $main (export "_start") ;; Test v128.not v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 v128.not v128.const i8x16 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 i8x16.eq call $assert_true ;; Test v128.and v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0 v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0 v128.and v128.const i8x16 255 255 0 0 0 0 0 0 255 255 0 0 0 0 0 0 i8x16.eq call $assert_true ;; Test v128.andnot v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0 v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0 v128.andnot v128.const i8x16 0 0 255 255 0 0 0 0 0 0 255 255 0 0 0 0 i8x16.eq call $assert_true ;; Test v128.or v128.const i8x16 255 255 0 0 0 0 255 255 255 255 0 0 0 0 255 0 v128.const i8x16 0 0 255 255 255 255 0 0 0 0 255 255 255 255 0 0 v128.or v128.const i8x16 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 0 i8x16.eq call $assert_true ;; Test v128.xor v128.const i8x16 255 255 0 0 255 255 0 0 255 255 0 0 255 255 0 0 v128.const i8x16 255 255 255 255 0 0 0 0 255 255 255 255 0 0 0 0 v128.xor v128.const i8x16 0 0 255 255 255 255 0 0 0 0 255 255 255 255 0 0 i8x16.eq call $assert_true i32.const 0 call $proc_exit ) ) ```

jammar1 commented Oct 1, 2024

View reviewed changes

loganek reviewed Oct 1, 2024

View reviewed changes

wenyongh reviewed Oct 5, 2024

View reviewed changes

jammar1 force-pushed the jamamr1/add_stubs_and_one_imp branch from 98af4ac to 3cd395e Compare October 14, 2024 14:17

wenyongh approved these changes Oct 15, 2024

View reviewed changes

jammar1 force-pushed the jamamr1/add_stubs_and_one_imp branch from 3cd395e to 77c66d4 Compare October 15, 2024 10:44

loganek approved these changes Oct 15, 2024

View reviewed changes

loganek reviewed Oct 15, 2024

View reviewed changes

James Marsh added 2 commits October 15, 2024 13:38

Add all SIMD operations into wasm_interp_fast switch

5b2ab61

jammar1 force-pushed the jamamr1/add_stubs_and_one_imp branch from 77c66d4 to c1bfe2a Compare October 15, 2024 12:50

loganek merged commit 2e61591 into bytecodealliance:dev/simd_for_interp Oct 15, 2024
383 checks passed

jammar1 deleted the jamamr1/add_stubs_and_one_imp branch October 15, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all SIMD operations into switch, implement V128 comparison operations. #3833

Add all SIMD operations into switch, implement V128 comparison operations. #3833

jammar1 commented Oct 1, 2024

jammar1 Oct 1, 2024

loganek Oct 15, 2024

jammar1 Oct 15, 2024

loganek Oct 1, 2024

jammar1 Oct 14, 2024

loganek Oct 15, 2024

loganek Oct 1, 2024

loganek Oct 1, 2024

wenyongh Oct 5, 2024

wenyongh left a comment

wenyongh Oct 15, 2024

jammar1 Oct 15, 2024

loganek left a comment

loganek Oct 15, 2024

loganek Oct 15, 2024

loganek Oct 15, 2024

		@@ -487,6 +487,8 @@ wasm_interp_get_frame_ref(WASMInterpFrame *frame)

		#define POP_F64() (GET_F64_FROM_ADDR(frame_lp + GET_OFFSET()))

		#define POP_V128() (GET_V128_FROM_ADDR(frame_lp + GET_OFFSET()))

Add all SIMD operations into switch, implement V128 comparison operations. #3833

Add all SIMD operations into switch, implement V128 comparison operations. #3833

Conversation

jammar1 commented Oct 1, 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

wenyongh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganek 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