Fix a bug if there is multiple return and one of them is in block #1063

lum1n0us · 2022-03-29T09:30:32Z

No description provided.

wenyongh · 2022-03-30T09:58:34Z

core/iwasm/fast-jit/fe/jit_emit_control.c

+    gen_commit_values(cc->jit_frame, cc->jit_frame->lp, cc->jit_frame->sp);
+    clear_values(cc->jit_frame);
+
+    insn = GEN_INSN(JMP, 0);


Do you want to make all "return opcodes" jump to the same func block's end?

YES, That is the plan. To avoid creating the same function return basic block multiple times.

Yes, an issue is that the function return values here are in current block, and haven't been copied to the dest block (block_func).
Seems that we needn't to gen_commit_values, just copy results and clear values:

SET_BB_END_BCIP(cc->cur_basic_block, *p_frame_ip - 1); copy_block_arities(cc, frame_sp_dst, block_func->result_types, block_func->result_count); clear_values(cc->jit_frame);

I thought the following end of the JitBlock will do that job

The end of JitBlock might not be executed:

(func result i32 block i32.const 1234 return i32.const 4567 end )

Another issue is about the performance, here it just copies the return values from current block to previous frame and returns to the caller. If jumping to func block, it needs to copy return values to that block, and in its end, copies values again to previous frame, which degrades the performance.
How about not changing it?

core/iwasm/fast-jit/fe/jit_emit_control.c

wenyongh · 2022-03-30T10:00:45Z

core/iwasm/fast-jit/fe/jit_emit_control.c

+    /* start a new basic block */
+    CREATE_BASIC_BLOCK(basic_block);
+    SET_BB_BEGIN_BCIP(basic_block, *p_frame_ip);
+    SET_BUILDER_POS(basic_block);


No need to create new basic block, it has no processor, no other code will jump to it.

Since the current basic block is closed with a JMP, I think we will need a new basic block to include the next IRs.

No need to create the new basic block, the next handle_next_reachable_block() will find a new jit block, create it's end or else basic block if it is not created, and then start to translate that basic block.

lum1n0us · 2022-04-01T06:34:47Z

The patch will prevent handle_next_reachable_block from consuming all JitBlock. But generated IRs still seems a litter wired.

  (func (export "e0") (param i32) (result i32)
    (i32.const 3)
    (if (param i32) (result i32) (local.get 0)
      (then
        (block (param i32) (result i32)
          (drop)
          (i32.const 7)
          (return)
        )
      )
      (else
        (return)
      )
    )
    (return)
  )

Generated IRs and some problems(Not sure if they are problems or not)

there will be two function return IRs in the end JitBasicBlock. The first one from return Opcode and second from handle_func_return in handle_op_end
should there be a jmp at the end of the else JitBasicBlock

;; skip L0
L2:
    ; PREDS(L0)
    ; BEGIN_BCIP=0x0021 END_BCIP=0x0026
    LDI32           i7, I0, 0x90
    STI32           0x3, I0, 0x94       ;-> gen_commit_value()
    CMP             i6, i7, 0x0         ;-> if (local.get 0) != 0
    BNE             i6, L3, L4          ;-> L3: if  L4: else
    ; SUCCS(L3 L4)

L3:
    ; PREDS(L2)
    ; BEGIN_BCIP=0x0027 END_BCIP=0x002e
    LDI32           i8, I0, 0x94        ;-> i32.const 3
    LDI64           I21, I0, 0x0        ;-> function return
    LDI64           I22, I21, 0x70
    STI32           0x7, I22, 0x0
    ADD             I22, I22, 0x4L
    STI64           I22, I21, 0x70
    STI64           I0, I15, 0xc90
    STI64           I21, I15, 0x68
    MOV             I0, I21
    RETURNBC        0x0, VOID, VOID
    STI32           0x7, I0, 0x94       ;-> gen_commit_value()
    JMP             L5                       ;-> end
    ; SUCCS(L5)

L4:
    ; PREDS(L2)
    ; BEGIN_BCIP=0x002f END_BCIP=0x0030
    LDI32           i9, I0, 0x94        ;-> i32.const 3
    LDI64           I23, I0, 0x0        ;-> function return
    LDI64           I24, I23, 0x70
    STI32           i9, I24, 0x0
    ADD             I24, I24, 0x4L
    STI64           I24, I23, 0x70
    STI64           I0, I15, 0xc90
    STI64           I23, I15, 0x68
    MOV             I0, I23
    RETURNBC        0x0, VOID, VOID
    STI32           i9, I0, 0x94        ;-> gen_commit_value()
                                                 ;-> miss a JMP ?
    ; SUCCS()

L5:
    ; PREDS(L3)
    ; BEGIN_BCIP=0x0031 END_BCIP=0x0033
    LDI32           i10, I0, 0x94       ;-> load_block_result()
    LDI64           I25, I0, 0x0        ;-> function return  from `return` opcode
    LDI64           I26, I25, 0x70
    STI32           i10, I26, 0x0
    ADD             I26, I26, 0x4L
    STI64           I26, I25, 0x70
    STI64           I0, I15, 0xc90
    STI64           I25, I15, 0x68
    MOV             I0, I25
    RETURNBC        0x0, VOID, VOID     ;-> function return from L408
    LDI64           I27, I0, 0x0
    LDI64           I28, I27, 0x70
    STI32           i10, I28, 0x0
    ADD             I28, I28, 0x4L
    STI64           I28, I27, 0x70
    STI64           I0, I15, 0xc90
    STI64           I27, I15, 0x68
    MOV             I0, I27
    RETURNBC        0x0, VOID, VOID
    ; SUCCS()
;; skip exception handlers
;; skip L1

wenyongh · 2022-04-02T09:37:31Z

core/iwasm/fast-jit/fe/jit_emit_control.c

            *p_frame_ip = block->wasm_code_end + 1;
-            return handle_op_end(cc, p_frame_ip, false);
+            return handle_op_end(
+                cc, p_frame_ip, block == jit_block_stack_top(&cc->block_stack));


Not sure why adding block == jit_block_stack_top(&cc->block_stack), for case below:

(module (type $0 (func (result i32))) (export "test" (func $1)) (func $1 (type $0) (result i32) i32.const 5678 (block $l (param i32) (result i32) i32.const 1234 i32.add i32.const 1 br 0 drop i32.const 666 ) ) )

Two 'JMP' instructions are generated in L2:

L2: ; PREDS(L0) ; BEGIN_BCIP=0x0027 END_BCIP=0x0038 ADD i7, 0x162e, 0x4d2 ADD I21, I0, 0x90L STI32 0x1, I21, 0x0 JMP L3 JMP L3 ; SUCCS(L3)

wenyongh · 2022-04-02T09:43:17Z

core/iwasm/fast-jit/fe/jit_emit_control.c

        else if (block->incoming_insns_for_end_bb) {
            *p_frame_ip = block->wasm_code_end + 1;
-            return handle_op_end(cc, p_frame_ip, false);
+            return handle_op_end(cc, p_frame_ip, block == first_block);


Why need to add block == first_block? For case below:

(module (type $0 (func (result i32))) (export "test" (func $1)) (func $1 (type $0) (result i32) i32.const 5678 (block $l (param i32) (result i32) i32.const 1234 i32.add i32.const 1 br 0 drop i32.const 666 ) ) )

Dumplicated JMP instruction are generated in L2:

L2: ; PREDS(L0) ; BEGIN_BCIP=0x0027 END_BCIP=0x0038 ADD i7, 0x162e, 0x4d2 ADD I21, I0, 0x90L STI32 0x1, I21, 0x0 JMP L3 JMP L3 ; SUCCS(L3)

try to handle one of the above problems. (else return end)

(func (export "e0") (param i32) (result i32) (i32.const 3) (if (param i32) (result i32) (local.get 0) (then (block (param i32) (result i32) (drop) (i32.const 7) (return) ) ) (else (return) ) ) (return) )

And missing jmp in ELSE branch

L4: ; PREDS(L2) ; BEGIN_BCIP=0x002f END_BCIP=0x0030 LDI32 i9, I0, 0x94 ;-> i32.const 3 LDI64 I23, I0, 0x0 ;-> function return LDI64 I24, I23, 0x70 STI32 i9, I24, 0x0 ADD I24, I24, 0x4L STI64 I24, I23, 0x70 STI64 I0, I15, 0xc90 STI64 I23, I15, 0x68 MOV I0, I23 RETURNBC 0x0, VOID, VOID STI32 i9, I0, 0x94 ;-> gen_commit_value() ;-> miss a JMP ? ; SUCCS()

Guess we need more condition

No need to jmp here，RETURNBC will return to caller, the following insns won't be executed. In fact, the STI32 should not be generated. Guess there are unneeded commit_values called.

core/iwasm/fast-jit/fe/jit_emit_control.c

here is a case: ``` wast (func (export "e0") (result i32) (block (i32.const 3) (return) ) (i32.const 7) (return) ) ```

wenyongh · 2022-04-02T13:11:04Z

core/iwasm/fast-jit/fe/jit_emit_control.c

 {
-    JitBlock *block = cc->block_stack.block_list_end, *block_prev;
+    JitBlock *block = jit_block_stack_top(&cc->block_stack);
+    JitBlock *first_block = jit_block_stack_top(&cc->block_stack);


Unused line, had better remove it

wenyongh · 2022-04-03T10:35:09Z

core/iwasm/fast-jit/fe/jit_emit_control.c

+                /* IF...ELSE... and return from ELSE */
+                else {
+                    *p_frame_ip = block->wasm_code_end + 1;
+                    return handle_op_end(cc, p_frame_ip, true);


Not sure whether this code causes the issue, for case below:

(module (func (export "test") (param i32) (result i32) (i32.const 3) (if (param i32) (result i32) (local.get 0) (then (block (param i32) (result i32) (drop) (i32.const 7) (return) ) ) (else (return) ) ) (return) ) )

Unnecessary INSNs are generated after RETURNBC:

L3: ; PREDS(L2) ; BEGIN_BCIP=0x0029 END_BCIP=0x0030 LDI32 i8, I0, 0x94 LDI64 I21, I0, 0x0 LDI64 I22, I21, 0x70 STI32 0x7, I22, 0x0 ADD I22, I22, 0x4L STI64 I22, I21, 0x70 STI64 I0, I15, 0xc90 STI64 I21, I15, 0x68 MOV I0, I21 RETURNBC 0x0, VOID, VOID STI32 0x7, I0, 0x94 ====> Unused INSN JMP L5 ====> Unused INSN ; SUCCS(L5) L4: ; PREDS(L2) ; BEGIN_BCIP=0x0031 END_BCIP=0x0032 LDI32 i9, I0, 0x94 LDI64 I23, I0, 0x0 LDI64 I24, I23, 0x70 STI32 i9, I24, 0x0 ADD I24, I24, 0x4L STI64 I24, I23, 0x70 STI64 I0, I15, 0xc90 STI64 I23, I15, 0x68 MOV I0, I23 RETURNBC 0x0, VOID, VOID STI32 i9, I0, 0x94 ====> Unused INSN JMP L5 ====> Unused INSN ; SUCCS(L5)

Yes I noticed that. Seems if if there is a return in a basic block, we should skip the end. But needs to take care cases of branches

Yes, I merged the patch firstly, let's test and fix if issues are found.

wenyongh · 2022-04-03T10:44:10Z

core/iwasm/fast-jit/fe/jit_emit_control.c

+                block = jit_block_stack_top(&cc->block_stack);
+            }
        }
+        /* Branches create a JMP*/


For below case:

(module (type $0 (func (param i32 i32) (result i32))) (export "test" (func $1)) (func $1 (type $0) (param $0 i32) (param $1 i32) (result i32) local.get $0 local.get $1 (if (param i32 i32) (result i32) (local.get $0) (then (i32.add) br 1 ) (else (i32.sub) local.get $1 br 1 ) ) ) )

Duplicated JMP is generated, and unnecessary LD/ST are generated:

L3: ; PREDS(L2) ; BEGIN_BCIP=0x002c END_BCIP=0x002f LDI32 i9, I0, 0x98 LDI32 i10, I0, 0x9c ADD i11, i9, i10 JMP L6 JMP L5 ====> Unnecessary JMP ; SUCCS(L5) L4: ; PREDS(L2) ; BEGIN_BCIP=0x0030 END_BCIP=0x0035 LDI32 i12, I0, 0x98 LDI32 i13, I0, 0x9c SUB i14, i12, i13 LDI32 i15, I0, 0x94 ADD I21, I0, 0x98L STI32 i15, I21, 0x0 JMP L6 JMP L5 ====> Unnecessary JMP L5: ; PREDS(L3 L4) ; BEGIN_BCIP=0x0036 END_BCIP=0x0036 LDI32 i16, I0, 0x98 STI32 i16, I0, 0x98 ==> Unnecessary LDI32 and STDI32 JMP L6 ; SUCCS(L6)

It is really not easy to fix the issues, I fix some issues and modify this file directly, some cases are tested successfully. If there are issues found, please help report and let's fix them together.

Fix issues of compiling control related opcodes (bytecodealliance#1063)

wenyongh reviewed Mar 30, 2022

View reviewed changes

lum1n0us force-pushed the dev/fast_jit branch from 39fb340 to 59ce8cd Compare April 1, 2022 06:26

lum1n0us force-pushed the dev/fast_jit branch 2 times, most recently from 1e8373f to e5b9124 Compare April 2, 2022 09:38

wenyongh reviewed Apr 2, 2022

View reviewed changes

lum1n0us force-pushed the dev/fast_jit branch 2 times, most recently from b562b2b to e826e2d Compare April 2, 2022 11:10

Fix a bug if there is a return in block

e826e2d

here is a case: ``` wast (func (export "e0") (result i32) (block (i32.const 3) (return) ) (i32.const 7) (return) ) ```

wenyongh reviewed Apr 3, 2022

View reviewed changes

Update jit_emit_control.c

21faa55

It is really not easy to fix the issues, I fix some issues and modify this file directly, some cases are tested successfully. If there are issues found, please help report and let's fix them together.

wenyongh merged commit 4d966d4 into bytecodealliance:dev/fast_jit Apr 3, 2022

wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Apr 3, 2022

Merge pull request #497 from bytecodealliance/dev/fast_jit

32fd358

Fix issues of compiling control related opcodes (bytecodealliance#1063)

lum1n0us deleted the dev/fast_jit branch May 10, 2022 14:41

Fix a bug if there is multiple return and one of them is in block #1063

Fix a bug if there is multiple return and one of them is in block #1063

Uh oh!

Conversation

lum1n0us commented Mar 29, 2022

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!

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!

lum1n0us commented Apr 1, 2022

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants