Implement Integer compare in wasm to x86 backend#1276
Implement Integer compare in wasm to x86 backend#1276certik merged 11 commits intolcompilers:mainfrom
Conversation
| 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) |
There was a problem hiding this comment.
This prints as:
$ lpython integration_tests/if_02.py --backend wasm_x86 && ./a.out
00I don't know, what's causing this issue!
There was a problem hiding this comment.
After the compare operation, eax wasn't pushed into the stack. This was causing the issue.
ed91c64 to
c647e2a
Compare
|
This PR is dependable on #1264 |
c647e2a to
8036eb9
Compare
|
Now, the compare statement is working properly! |
| } | ||
|
|
||
| void handle_I32Compare(const std::string &compare_op) { | ||
| unique_id.push_back(std::to_string(offset)); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, Instead of storing the unique_id, we can just use std::to_string(offset)
Yup, we can just use offset as a label.
There was a problem hiding this comment.
I incorporated this change into this commit WASM_X86: Use offset directly as label
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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));There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I added this change/improvement into this commit
Refactor: WASM_X86: Directly push value onto stack.
|
Thank you for updating the changes. |
|
Thank you for your suggestions and guidance on them. They were really helpful. |
Related: #1222