Implement if-else in wasm_to_x86 backend by Thirumalai-Shaktivel · Pull Request #1264 · lcompilers/lpython

Thirumalai-Shaktivel · 2022-11-04T09:42:58Z

Related #1222

Thirumalai-Shaktivel · 2022-11-04T09:46:28Z

+        while (cur_byte != 0x0B) {
+            if (cur_byte == 0x05) {
+                cur_byte = wasm::read_b8(code, offset_);
+                if (cur_byte == 0x0B || cur_byte == 0x41) {


I have a doubt:
Does wasm always have an else block (even though else is empty) in the if statement?

@Shaikh-Ubaid
If so, how Assert is handled in asr_to_wasm?

I asked this because, the else block is not empty in the visit_assert(), it has 0x41

Does wasm always have an else block (even though else is empty) in the if statement?

Currently, if there is no else block, then in the wat generated, we would just have if <some instructions> end.

I asked this because, the else block is not empty in the visit_assert(), it has 0x41

Do you mean visit_Assert() in ASR->WASM backend?

Nah, After the wasm code is generated and captured in the wasm->x86

Thirumalai-Shaktivel · 2022-11-04T14:23:42Z

+        while (cur_byte != 0x0B) {
+            if (cur_byte == 0x05) {
+                cur_byte = wasm::read_b8(code, offset_);
+                if (cur_byte == 0x0B || cur_byte == 0x41) {
+                    return false;
+                } else {
+                    return true;
+                }
+            }
+            cur_byte = wasm::read_b8(code, offset_);
+        }


But this logic, detects the else block, even for the following syntax!!

if True: pass

That is the reason, I check the next byte of the visit_else to be 0x0B (which checks the end of the if stmt).

At present, if there is no else block, then visit_Else() should not be visited. I guess, we might be missing something.

ubaidsk · 2022-11-04T15:58:22Z

I looked into the PR. It seems, it would be better if wasm backend adds an else block always (it would be empty when it is not needed).

If an else is always present, then there would not be any need of checking if any else is present (since, it would be always present (although empty when not needed)).

ubaidsk · 2022-11-04T16:00:54Z

I will rebase this on top of #1262 (so that we can print some value inside the if-else ladder), make the necessary updates to the wasm backend and push to this PR.

Thirumalai-Shaktivel · 2022-11-04T16:38:38Z

This looks great! Thanks for updating.

ubaidsk · 2022-11-04T16:44:26Z

I will rebase this on top of #1262

I rebased on top of latest main. Rebasing on top of #1262 is including many commits of that PR (it also makes this PR dependent on it).

Thirumalai-Shaktivel · 2022-11-05T07:46:07Z

Let's implement visit_Assert() in this PR itself.
Assertion in wasm_x86, behaves differently!

def Main0():
    assert 1 == 1

Main0()

Output:

$ lpython examples/expr2.py --backend wasm_x86 && ./a.out
AssertionError

def Main0():
    assert 0 == 1

Main0()

$ lpython examples/expr2.py --backend wasm_x86 && ./a.out

ubaidsk · 2022-11-05T16:21:31Z

    void visit_Assert(const ASR::Assert_t &x) {
        this->visit_expr(*x.m_test);
-        wasm::emit_i32_eqz(m_code_section, m_al);
        wasm::emit_b8(m_code_section, m_al, 0x04);  // emit if start
        wasm::emit_b8(m_code_section, m_al, 0x40);  // empty block type
+        wasm::emit_b8(m_code_section, m_al, 0x05);  // starting of else
        if (x.m_msg) {
            std::string msg =
                ASR::down_cast<ASR::StringConstant_t>(x.m_msg)->m_s;


The previous version was more correct, right? The previous version would say -> If Assert condition failed, raise an error (which is what assert does, I think).

The main motive for these changes was that visit_I32Eqz() in the wasm_to_x86 is not working correctly.
Here, the changes imply:

If True: // Do nothing else: print("AssertionError") quit(1)

which is similar to assert False

The same approach as we do in the LLVM backend:

lpython/src/libasr/codegen/asr_to_llvm.cpp

Lines 4845 to 4862 in d4a6d53

void visit_Assert(const ASR::Assert_t &x) {

this->visit_expr_wrapper(x.m_test, true);

create_if_else(tmp, []() {}, [=]() {

if (x.m_msg) {

char* s = ASR::down_cast<ASR::StringConstant_t>(x.m_msg)->m_s;

llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError: %s\n");

llvm::Value *fmt_ptr2 = builder->CreateGlobalStringPtr(s);

print_error(context, *module, *builder, {fmt_ptr, fmt_ptr2});

} else {

llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError\n");

print_error(context, *module, *builder, {fmt_ptr});

}

int exit_code_int = 1;

llvm::Value *exit_code = llvm::ConstantInt::get(context,

llvm::APInt(32, exit_code_int));

exit(context, *module, *builder, exit_code);

});

}

ubaidsk · 2022-11-05T16:24:45Z

        }
        wasm::emit_i32_const(m_code_section, m_al, 1);  // non-zero exit code
        exit();
-        wasm::emit_b8(m_code_section, m_al, 0x05);  // starting of else
        wasm::emit_expr_end(m_code_section, m_al);  // emit if end
    }
 };


The else portion was empty in the previous version.

ubaidsk

The right way to support Assert would instead be to implement the visit_I32Eqz in the WASM->X86 backend.

void visit_I32Eqz() {
        m_a.asm_pop_r32(X86Reg::eax);
        m_a.asm_mov_r32_imm32(X86Reg::ebx, 0U);
        m_a.asm_cmp_r32_r32(X86Reg::eax, X86Reg::ebx);
        m_a.asm_push_r32(X86Reg::eax);
    }

certik · 2022-11-05T20:56:17Z

Thanks @Thirumalai-Shaktivel for working on this, very helpful. Thanks @Shaikh-Ubaid for the review.

Thirumalai-Shaktivel · 2022-11-06T07:48:00Z

The right way to support Assert would instead be to implement the visit_I32Eqz in the WASM->X86 backend.
void visit_I32Eqz() {
        m_a.asm_pop_r32(X86Reg::eax);
        m_a.asm_mov_r32_imm32(X86Reg::ebx, 0U);
        m_a.asm_cmp_r32_r32(X86Reg::eax, X86Reg::ebx);
        m_a.asm_push_r32(X86Reg::eax);
    }

This seems not working in the wasm_x86 backend.

Thirumalai-Shaktivel · 2022-11-06T07:54:38Z

What does I32Eqz do? isn't it a NOT operator?

ubaidsk · 2022-11-06T16:21:37Z

What does I32Eqz do? isn't it a NOT operator?

It checks if the stack top is equal to zero. Yes, it works similar to a not operator.

The same approach as we do in the LLVM backend:

lpython/src/libasr/codegen/asr_to_llvm.cpp

Lines 4845 to 4862 in d4a6d53

void visit_Assert(const ASR::Assert_t &x) {

this->visit_expr_wrapper(x.m_test, true);

create_if_else(tmp, []() {}, [=]() {

if (x.m_msg) {

char* s = ASR::down_cast<ASR::StringConstant_t>(x.m_msg)->m_s;

llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError: %s\n");

llvm::Value *fmt_ptr2 = builder->CreateGlobalStringPtr(s);

print_error(context, *module, *builder, {fmt_ptr, fmt_ptr2});

} else {

llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError\n");

print_error(context, *module, *builder, {fmt_ptr});

}

int exit_code_int = 1;

llvm::Value *exit_code = llvm::ConstantInt::get(context,

llvm::APInt(32, exit_code_int));

exit(context, *module, *builder, exit_code);

});

}

If it is similar to the LLVM backend, we can follow it.

void visit_I32Eqz() {
        m_a.asm_pop_r32(X86Reg::eax);
        m_a.asm_mov_r32_imm32(X86Reg::ebx, 0U);
        m_a.asm_cmp_r32_r32(X86Reg::eax, X86Reg::ebx);
        m_a.asm_push_r32(X86Reg::eax);
}.

This seems not working in the wasm_x86 backend

It should (hopefully) work. I tested it locally on my system before sharing here. If we are following the LLVM approach, then we might not need it (I guess) and we can add it later when needed.

Thirumalai-Shaktivel · 2022-11-06T16:37:42Z

It should (hopefully) work. I tested it locally on my system before sharing here.

Were you able to get "AssertionError" for assert False?
This is weird, it didn't work for me!

Thirumalai-Shaktivel · 2022-11-06T16:43:39Z

For the code:

def Test_assert_01():
    if True:
        assert True

    assert 1 != 1, "One is equal to one."

def Verify():
    Test_assert_01()

Verify()

I get:

$ lpython examples/expr2.py --backend wasm_x86 && ./a.out
Call to imported function with index 4 not yet supported
Call to imported function with index 4 not yet supported

Thirumalai-Shaktivel · 2022-11-06T16:45:41Z

Sorry my bad, print_str doesn't work yet in this PR! I will rebase that PR and report back.

ubaidsk · 2022-11-06T16:49:41Z

Sorry my bad, print_str doesn't work yet in this PR! I will rebase that PR and report back.

Let's wait for the PRs to be merged. It seems many PRs are interdependent. Let's work on Assertion in another PR once all the necessary PRs are merged. For Assertion we would also be needing support for set_exit_code() (since, if the assertion failed, we should exit with code 1). It would be better if we support Assert separately, I think.

Thirumalai-Shaktivel · 2022-11-06T16:53:00Z

Yup! Now, I feel the same.
I will submit another PR for Assert once the print_str is merged

Thirumalai-Shaktivel · 2022-11-06T16:56:47Z

Thank you for your approval and guidance!

certik · 2022-11-10T18:14:07Z

   public:
    X86Assembler &m_a;
    uint32_t cur_func_idx;
+    std::vector<uint32_t> unique_id;


Later:

Suggested change

std::vector<uint32_t> unique_id;

uint32_t unique_id;

certik · 2022-11-10T18:15:09Z

+    void visit_EmtpyBlockType() {}
+
+    void visit_If() {
+        unique_id.push_back(offset);


Suggested change

unique_id.push_back(offset);

unique_id++;

uint32_t label = unique_id;

certik · 2022-11-10T18:15:15Z

+        m_a.asm_cmp_r32_imm8(LFortran::X86Reg::eax, 1);
+        m_a.asm_je_label(".then_" + std::to_string(unique_id.back()));
+        m_a.asm_jmp_label(".else_" + std::to_string(unique_id.back()));
+        m_a.add_label(".then_" + std::to_string(unique_id.back()));


For later (but probably quite soon): we should not be using strings for labels, but just unique integers. The unique integer can just be a counter in this visitor, just an integer, see above.

(we might need to have a stack of unique ids...)

certik

Looks good to me, thanks!

Thirumalai-Shaktivel added the wasm_x86 label Nov 4, 2022

Thirumalai-Shaktivel requested review from certik and ubaidsk November 4, 2022 09:42

Thirumalai-Shaktivel commented Nov 4, 2022

View reviewed changes

Thirumalai-Shaktivel mentioned this pull request Nov 4, 2022

WASM to X86 Backend #1222

Closed

Thirumalai-Shaktivel force-pushed the if_stmt branch from d494fb3 to 7b8e83f Compare November 4, 2022 15:05

ubaidsk force-pushed the if_stmt branch from 7b8e83f to 1f15022 Compare November 4, 2022 16:30

Thirumalai-Shaktivel and others added 5 commits November 4, 2022 22:04

X86: Support diagnostics in asr_to_x86()

19309b8

WASM: Emit else always

2aea75b

WASM_X86: Implement Visit if-else

03fbb06

WASM_X86: Minor debugging help

8fb624b

TEST: WASM_X86: Add test for if-else

4b09191

ubaidsk added 2 commits November 4, 2022 22:10

TEST: WASM_X86: Add another test for if-else

bfa5887

TEST: Update reference tests

4093db1

ubaidsk force-pushed the if_stmt branch from 1f15022 to 4093db1 Compare November 4, 2022 16:41

ubaidsk approved these changes Nov 4, 2022

View reviewed changes

ubaidsk reviewed Nov 5, 2022

View reviewed changes

ubaidsk requested changes Nov 5, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the if_stmt branch from 6e47f8e to 4093db1 Compare November 6, 2022 16:54

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

ubaidsk approved these changes Nov 6, 2022

View reviewed changes

Thirumalai-Shaktivel mentioned this pull request Nov 9, 2022

Implement Integer compare in wasm to x86 backend #1276

Merged

certik reviewed Nov 10, 2022

View reviewed changes

certik approved these changes Nov 10, 2022

View reviewed changes

certik merged commit 272951e into lcompilers:main Nov 10, 2022

Thirumalai-Shaktivel deleted the if_stmt branch November 11, 2022 07:51

Thirumalai-Shaktivel mentioned this pull request Nov 14, 2022

Implement Loop statement #1290

Merged

ubaidsk mentioned this pull request Jan 2, 2023

WASM_X64: Support if else #1390

Merged

	void visit_Assert(const ASR::Assert_t &x) {
	this->visit_expr_wrapper(x.m_test, true);
	create_if_else(tmp, []() {}, [=]() {
	if (x.m_msg) {
	char* s = ASR::down_cast<ASR::StringConstant_t>(x.m_msg)->m_s;
	llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError: %s\n");
	llvm::Value *fmt_ptr2 = builder->CreateGlobalStringPtr(s);
	print_error(context, module, builder, {fmt_ptr, fmt_ptr2});
	} else {
	llvm::Value *fmt_ptr = builder->CreateGlobalStringPtr("AssertionError\n");
	print_error(context, module, builder, {fmt_ptr});
	}
	int exit_code_int = 1;
	llvm::Value *exit_code = llvm::ConstantInt::get(context,
	llvm::APInt(32, exit_code_int));
	exit(context, module, builder, exit_code);
	});
	}

	unique_id.push_back(offset);
	unique_id++;
	uint32_t label = unique_id;

Uh oh!

Conversation

Thirumalai-Shaktivel commented Nov 4, 2022

Uh oh!

Thirumalai-Shaktivel Nov 4, 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 4, 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!

Thirumalai-Shaktivel Nov 4, 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!

ubaidsk commented Nov 4, 2022

Uh oh!

ubaidsk commented Nov 4, 2022

Uh oh!

Thirumalai-Shaktivel commented Nov 4, 2022

Uh oh!

ubaidsk commented Nov 4, 2022

Uh oh!

Thirumalai-Shaktivel commented Nov 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 6, 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!

ubaidsk left a comment • 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!

certik commented Nov 5, 2022

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ubaidsk commented Nov 6, 2022

Uh oh!

Thirumalai-Shaktivel commented Nov 6, 2022

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ubaidsk commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Nov 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel Nov 4, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 4, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 4, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 6, 2022 •

edited

Loading

ubaidsk left a comment •

edited

Loading

Thirumalai-Shaktivel commented Nov 6, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 6, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 6, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 6, 2022 •

edited

Loading

ubaidsk commented Nov 6, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 6, 2022 •

edited

Loading