Add the tail-call feature support for classic-interp by qinxk-inter · Pull Request #8 · qinxk-inter/wasm-micro-runtime

qinxk-inter · 2020-09-22T08:25:06Z

fast-interp is still WIP

xujuntwt95329 · 2020-09-22T11:08:49Z

                }

                func->has_op_func_call = true;
+                if (opcode == WASM_OP_RETURN_CALL_INDIRECT) { goto handle_op_return; }


Same with the previous comment. Need to set a flag to avoid emitting operand when build as fast-interpreter

And same in wasm_loader.c

Postpone this part to fast-interpreter implementation.

xujuntwt95329 · 2020-09-22T11:13:44Z

+    FREE_FRAME(exec_env, frame);
+    word_copy(frame->lp, frame_sp, cur_func->param_cell_num);


This works, but logically, we had better FREE_FRAME after word_copy

Suggested change

FREE_FRAME(exec_env, frame);

word_copy(frame->lp, frame_sp, cur_func->param_cell_num);

word_copy(frame->lp, frame_sp, cur_func->param_cell_num);

FREE_FRAME(exec_env, frame);

FREE_FRAME() first is because the tail call logic, pop the stack frame then call. More clear way may be FREE_FRAME(), outs_area to get stack_top and then word_copy() to it. But actually the outs_area operations could be saved by using current frame directly, and this is also suggested by @wenyongh in ppt. :) So less codes or clear logic?

Yes, we should use frame->lp directly rather than get outs_area.

My concern here is: the frame->lp is referenced after FREE_FRAME(exec_env, frame), we know this will work just because we know FREE_FRAME will not modify the content in the memory, but how about we don't know? Just like dereference a freed pointer free(ptr); a = *ptr;. On most platforms free will not set ptr to NULL, but we never do this.

How do you think of this? @wenyongh

xujuntwt95329 · 2020-09-22T11:19:30Z

 #if WASM_ENABLE_FAST_INTERP != 0
                    /* emit the offset after return opcode */
                    POP_OFFSET_TYPE(ret_type);
 #endif


In fast-interpreter mode, when we jump into this branch from WASM_OP_RETURN_CALL the POP_OFFSET_TYPE(ret_type) will emit operand, which will make the generated code incorrect.

Maybe we need a flag to avoid POP_OFFSET_TYPE(ret_type) if we're checking result of tail call:

Suggested change

#if WASM_ENABLE_FAST_INTERP != 0

/* emit the offset after return opcode */

POP_OFFSET_TYPE(ret_type);

#endif

#if WASM_ENABLE_FAST_INTERP != 0

if (!tail_call_return_check)

/* emit the offset after return opcode */

POP_OFFSET_TYPE(ret_type);

#endif

and reset the flag after the switch block

Postpone this part to fast-interpreter implementation.

Sign in to view

+            if (cur_func->is_import_func
+#if WASM_ENABLE_MULTI_MODULE != 0
+            && !cur_func->import_func_inst
+#endif
+            )


Sign in to view

    WASM_OP_CALL          = 0x10, /* call */
    WASM_OP_CALL_INDIRECT = 0x11, /* call_indirect */
+    WASM_OP_RETURN_CALL   = 0x12, /* return_call */
+    WASM_OP_RETURN_CALL_INDIRECT   = 0x13, /* return_call_indirect */


Sign in to view

          goto call_func_from_interp;
        }

+      HANDLE_OP (WASM_OP_RETURN_CALL_INDIRECT):


Sign in to view

    HANDLE_OP (WASM_OP_UNUSED_0x08):
    HANDLE_OP (WASM_OP_UNUSED_0x09):
    HANDLE_OP (WASM_OP_UNUSED_0x0a):
-    HANDLE_OP (WASM_OP_UNUSED_0x12):


Sign in to view

                }

                func->has_op_func_call = true;
+                if (opcode == WASM_OP_RETURN_CALL) { goto handle_op_return; }


Sign in to view


            case WASM_OP_RETURN:
            {
-                int32 idx;


Sign in to view

            }

            case WASM_OP_CALL_INDIRECT:
+            case WASM_OP_RETURN_CALL_INDIRECT:


xujuntwt95329 · 2020-09-23T09:41:04Z

+            wasm_set_exception(module, "unknown function");
+            goto got_exception;


seems this file is 2 spaces indented

xujuntwt95329 · 2020-09-23T09:41:22Z

-
+#if WASM_ENABLE_TAIL_CALL != 0
+          if (opcode == WASM_OP_RETURN_CALL_INDIRECT)
+              goto call_func_from_return_call;


same as above

Signed-off-by: Xiaokang Qin <xiaokang.qxk@antgroup.com>

* add CI for tail call and custom name section * update CI for mac

xujuntwt95329 reviewed Sep 22, 2020

View reviewed changes

wenyongh reviewed Sep 23, 2020

View reviewed changes

qinxk-inter force-pushed the tail_call branch from 7d05f63 to f182330 Compare September 23, 2020 08:32

qinxk-inter marked this pull request as draft September 23, 2020 08:36

qinxk-inter force-pushed the tail_call branch 2 times, most recently from a28deff to 96ebc37 Compare September 23, 2020 08:55

qinxk-inter marked this pull request as ready for review September 23, 2020 09:01

xujuntwt95329 reviewed Sep 23, 2020

View reviewed changes

qinxk-inter force-pushed the tail_call branch from 96ebc37 to bcbe6a9 Compare September 23, 2020 09:52

qinxk-inter marked this pull request as draft September 23, 2020 09:54

qinxk-inter marked this pull request as ready for review September 23, 2020 10:06

qinxk-inter marked this pull request as draft September 23, 2020 10:19

qinxk-inter force-pushed the tail_call branch from bcbe6a9 to c23b603 Compare September 23, 2020 12:03

Add the tail-call feature support for classic-interp

c23b603

Signed-off-by: Xiaokang Qin <xiaokang.qxk@antgroup.com>

qinxk-inter marked this pull request as ready for review September 23, 2020 12:11

add CI for tail call and custom name section (#9)

ec4c965

* add CI for tail call and custom name section * update CI for mac

		FREE_FRAME(exec_env, frame);
		word_copy(frame->lp, frame_sp, cur_func->param_cell_num);

		wasm_set_exception(module, "unknown function");
		goto got_exception;

Conversation

qinxk-inter commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qinxk-inter commented Sep 22, 2020 •

edited

Loading