Implement Integer compare in wasm to x86 backend #1276

Thirumalai-Shaktivel · 2022-11-08T05:30:04Z

Related: #1222

Thirumalai-Shaktivel · 2022-11-08T05:32:21Z

integration_tests/if_02.py

+    if y != 1:
+        print(0)
+    else:
+        print(1)
+    # if y <= 1:
+    #     print(1)
+
+    # y = 5
+    # if y <= 10:
+    #     print(1)
+    # if y >= 5:
+    #     print(1)
+    # if y >= 1:
+    #     print(1)
+    print(y)


This prints as:

$ lpython integration_tests/if_02.py --backend wasm_x86 && ./a.out 00

I don't know, what's causing this issue!

After the compare operation, eax wasn't pushed into the stack. This was causing the issue.

…nique_id

Thirumalai-Shaktivel · 2022-11-09T16:35:31Z

This PR is dependable on #1264

Thirumalai-Shaktivel · 2022-11-09T16:38:24Z

Now, the compare statement is working properly!
I will start working on the loop statement.

certik

Looks good!

ubaidsk · 2022-11-11T07:00:01Z

src/libasr/codegen/wasm_to_x86.cpp

@@ -154,6 +154,41 @@ class X86Visitor : public WASMDecoder<X86Visitor>,
        m_a.asm_push_r32(X86Reg::eax);
    }

+    void handle_I32Compare(const std::string &compare_op) {
+        unique_id.push_back(std::to_string(offset));


@Thirumalai-Shaktivel, please, could you possibly share why we need to store the offset in unique_id? Are we using it later outside the handleI32Compare() function?

If it is not used later, we need/should not store it, I think.

Yup, I forgot to pop the uniqu_id!
Now, If I think of it, we have options:
1.

diff --git a/src/libasr/codegen/wasm_to_x86.cpp b/src/libasr/codegen/wasm_to_x86.cpp index cf968d5c4..12ea3b156 100644 --- a/src/libasr/codegen/wasm_to_x86.cpp +++ b/src/libasr/codegen/wasm_to_x86.cpp @@ -199,30 +199,29 @@ class X86Visitor : public WASMDecoder<X86Visitor>, } void handle_I32Compare(const std::string &compare_op) { - unique_id.push_back(std::to_string(offset)); m_a.asm_pop_r32(X86Reg::ebx); m_a.asm_pop_r32(X86Reg::eax); m_a.asm_cmp_r32_r32(X86Reg::eax, X86Reg::ebx); if (compare_op == "Eq") { - m_a.asm_je_label(".compare_1" + unique_id.back()); + m_a.asm_je_label(".compare_1" + std::to_string(offset)); ...

diff --git a/src/libasr/codegen/wasm_to_x86.cpp b/src/libasr/codegen/wasm_to_x86.cpp index cf968d5c4..2e23d7230 100644 --- a/src/libasr/codegen/wasm_to_x86.cpp +++ b/src/libasr/codegen/wasm_to_x86.cpp @@ -224,6 +224,7 @@ class X86Visitor : public WASMDecoder<X86Visitor>, m_a.asm_mov_r32_imm32(X86Reg::eax, 1); m_a.add_label(".compare.end_" + unique_id.back()); m_a.asm_push_r32(X86Reg::eax); + unique_id.pop_back(); } void visit_I32Eq() { handle_I32Compare("Eq"); }

which do you prefer the most?

As you said, yup, it is not used later on. So, Instead of storing the unique_id, we can just use std::to_string(offset). I think option 1 would be good to go.

Also, the reason why I used the unique_id is that: I thought the offset value would be incremented inside the handle_compareI32() function. I checked and it seems the offset is not modified. So, we can use std::to_string(offset).
I will make the able change with the loop stmt. Thanks for noticing it.

So, Instead of storing the unique_id, we can just use std::to_string(offset)

Yup, we can just use offset as a label.

I incorporated this change into this commit WASM_X86: Use offset directly as label

Thirumalai-Shaktivel · 2022-11-11T07:39:09Z

src/libasr/codegen/wasm_to_x86.cpp

+        m_a.asm_mov_r32_imm32(X86Reg::eax, 0);
+        m_a.asm_jmp_label(".compare.end_" + unique_id.back());
+        m_a.add_label(".compare_1" + unique_id.back());
+        m_a.asm_mov_r32_imm32(X86Reg::eax, 1);
+        m_a.add_label(".compare.end_" + unique_id.back());
+        m_a.asm_push_r32(X86Reg::eax);


In the last discussion, you said we can push value directly into the stack.
Here why not we just push the result value into the stack instead of moving it into the eax?
Also, I have another doubt:
How do we access the pushed value in the stack?

Yes, the other instructions that follow use it. For example, After the I32Const, there could be an instruction I32Neg (hypothetical example) which would use the value on top of stack (it pops it to use it), negate it and then push back onto stack.
#1222 (comment)

How would we use the value on top of stack?

Pushing the value directly into the stack works:

m_a.asm_push_imm8(0); m_a.asm_jmp_label(".compare.end_" + std::to_string(offset)); m_a.add_label(".compare_1" + std::to_string(offset)); m_a.asm_push_imm8(1); m_a.add_label(".compare.end_" + std::to_string(offset));

How do we access the pushed value in the stack?

It can be accessed during run-time only.

How would we use the value on top of stack?

Other instructions can use it during run-time time. They perform some operation(s) on it and store the result back onto stack.

Pushing the value directly into the stack works:

If it works, we can use this way. (I think it could be better as it could use (slightly) lesser instructions)

I added this change/improvement into this commit
Refactor: WASM_X86: Directly push value onto stack.

Thirumalai-Shaktivel · 2022-11-11T12:27:31Z

Thank you for updating the changes.

ubaidsk · 2022-11-11T12:35:28Z

Thank you for your suggestions and guidance on them. They were really helpful.

Thirumalai-Shaktivel added the wasm_x86 label Nov 8, 2022

Thirumalai-Shaktivel commented Nov 8, 2022

View reviewed changes

Thirumalai-Shaktivel and others added 8 commits November 8, 2022 11:03

X86: Support diagnostics in asr_to_x86()

c6a1c23

WASM: Emit else always

e36a105

WASM_X86: Implement Visit if-else

e2a67dc

WASM_X86: Minor debugging help

289f4a5

TEST: WASM_X86: Add test for if-else

e42dd24

TEST: WASM_X86: Add another test for if-else

46e2df0

TEST: Update reference tests

5cefa66

[WASM_x86] Convert the offset into strings and store it to use as a u…

0f1a064

…nique_id

Thirumalai-Shaktivel force-pushed the compare branch 3 times, most recently from ed91c64 to c647e2a Compare November 9, 2022 08:07

[WASM_x86] Implement Integer compare

66f6b0a

Add tests

8036eb9

Thirumalai-Shaktivel force-pushed the compare branch from c647e2a to 8036eb9 Compare November 9, 2022 16:36

Thirumalai-Shaktivel marked this pull request as ready for review November 9, 2022 16:36

Thirumalai-Shaktivel added the ready for review PRs that are ready for review label Nov 9, 2022

Merge branch 'main' into compare

dd7213b

certik approved these changes Nov 10, 2022

View reviewed changes

certik enabled auto-merge (squash) November 10, 2022 18:26

certik merged commit b157290 into lcompilers:main Nov 10, 2022

ubaidsk reviewed Nov 11, 2022

View reviewed changes

Thirumalai-Shaktivel commented Nov 11, 2022

View reviewed changes

Thirumalai-Shaktivel deleted the compare branch November 11, 2022 07:50

Implement Integer compare in wasm to x86 backend #1276

Implement Integer compare in wasm to x86 backend #1276

Uh oh!

Conversation

Thirumalai-Shaktivel commented Nov 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel commented Nov 9, 2022

Uh oh!

Thirumalai-Shaktivel commented Nov 9, 2022

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Thirumalai-Shaktivel Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Thirumalai-Shaktivel commented Nov 11, 2022

Uh oh!

ubaidsk commented Nov 11, 2022

Uh oh!

Uh oh!

Thirumalai-Shaktivel Nov 9, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 11, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 11, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 11, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 11, 2022 •

edited

Loading