Implement Loop statement #1290

Thirumalai-Shaktivel · 2022-11-12T07:30:04Z

No description provided.

Thirumalai-Shaktivel · 2022-11-12T07:37:24Z

src/libasr/codegen/wasm_to_x86.cpp

+    void visit_Br(uint32_t /*label_index*/) {
+        // Branch is used to jump to the `loop.head`.
+        m_a.asm_jmp_label(".loop.head_" + unique_id.rbegin()[1]);
+    }
+


What is the use of branch?
@Shaikh-Ubaid

I see, the label_index is not constant, it changes based on the statements inside the loop.

What is the use of branch?

It is like a jump instruction. Everytime we wish to continue a loop in wasm, we need to branch (only if the loop condition is true) to the loop.

https://samrat.me/posts/2020-03-29-webassembly-control-instr-examples/ offers a good explanation.

I see, the label_index is not constant, it changes based on the statements inside the loop.

Yes, the label_index is used as the target of the branch.

lpython/src/libasr/codegen/asr_to_wasm.cpp

Lines 2025 to 2029 in ce4612d

// From WebAssembly Docs:

// Unlike with other index spaces, indexing of labels is relative by

// nesting depth, that is, label 0 refers to the innermost structured

// control instruction enclosing the referring branch instruction, while

// increasing indices refer to those farther out.

As shared in the above comment, the label index counts from the inner most block (A block could be a loop or an if-else construct). In our case, a label_index of 0 points to the if-else condition (which is the loop condition) and a label_index of 1 points to the actual loop.

Also, on branching to a loop, the control passes to the beginning of the loop, where as on branching to an if block, the control passes to the end of it.

// Unlike with other index spaces, indexing of labels is relative by
// nesting depth, that is, label 0 refers to the innermost structured
// control instruction enclosing the referring branch instruction, while
// increasing indices refer to those farther out.

The following example should (hopefully) explain the above more clearly.

// Code in some dummy language, (might) not be exactly in `WebAssembly Text Format` (loop // label index = 3 (if // label index = 2 (if // label index = 1 (if // label index = 0 br some_label_index ) ) ) )

Depending on where we would like to branch, we can have appropriate value for some_label_index.

I got it, great explanation. Thanks

certik

This is great. @Shaikh-Ubaid you should still review this.

Thirumalai-Shaktivel · 2022-11-13T05:41:32Z

Thanks for reviewing and merging this PR

ubaidsk · 2022-11-13T08:37:16Z

@Shaikh-Ubaid you should still review this.

Yes, I will review this in the evening today.

ubaidsk · 2022-11-13T19:52:24Z

src/libasr/codegen/wasm_to_x86.cpp

+        /*
+        The loop statement starts with `loop.head`. The `loop.body` and
+        `loop.branch` are enclosed within the `if.block`. If the condition
+        fails, the loop is exited through `else.block`.
+        .head
+            .If
+                # Statements
+                .Br
+            .Else
+            .endIf
+        .end
+        */


Great note, Thirumalai !. It explains the loop structure very beautifully. Thank you so much for it.

ubaidsk · 2022-11-13T20:32:33Z

src/libasr/codegen/wasm_to_x86.cpp

+        if (if_unique_id.size() - loop_unique_id.size() == label_index - 1) {
+            // cycle/continue or loop.end
+            m_a.asm_jmp_label(".loop.head_" + loop_unique_id.back());
+        } else {
+            // exit/break
+            m_a.asm_jmp_label(".loop.end_" + loop_unique_id.back());
+        }


This if-else condition inside the visit_Br() is not clear to me. @Thirumalai-Shaktivel, please, could you possibly share how this if-else works and chooses whether to iterate the loop or end it?

Consider this example:

while i < 10: j = 0 while j < 10: k += 1 if i == 0: if j == 3: continue else: j += 1 j += 1 i += 1 assert k == 95

Output printed using:

diff --git a/src/libasr/codegen/wasm_to_x86.cpp b/src/libasr/codegen/wasm_to_x86.cpp index 9db9d2b4b..48f947076 100644 --- a/src/libasr/codegen/wasm_to_x86.cpp +++ b/src/libasr/codegen/wasm_to_x86.cpp @@ -131,6 +131,9 @@ class X86Visitor : public WASMDecoder<X86Visitor>, void visit_EmtpyBlockType() {} void visit_Br(uint32_t label_index) { +std::cout << "label_index: " << label_index << "\n"; +std::cout << "Loop_size: " << loop_unique_id.size() << "\n"; +std::cout << "If_size: " << if_unique_id.size() << "\n"; // Branch is used to jump to the `loop.head` or `loop.end`.

$ lpython examples/expr2.py --backend wasm_x86 && ./expr2.out label_index: 3 Loop_size: 2 If_size: 4 label_index: 1 Loop_size: 2 If_size: 2 label_index: 1 Loop_size: 1 If_size: 1

We have 2 loops and 2 if's. But in the backend, we get 2 loops and 4 ifs, right?
The first br comes from the continue, if we subtract 4 if's with 2 loops: we get the actual 2 if's. the label_index is 3, we do 3 - 1 = 2: jump to head and So on...

But if the break is encountered the label_index is 2, So we jump to the end of the loop
Any doubts, please ask

how this if-else works and chooses whether to iterate the loop or end it?

Every time, the loop is iterated using the Br. Also, I didn't analyze how the loop is being exited. I will look into it soon.

I see, the Loop is exited through the else block:

.loop.head_297: mov eax, [ebp-4] push eax push 0x0000000a pop ebx pop eax cmp eax, ebx jl .compare_1303 push 0x00 jmp .compare.end_303 .compare_1303: push 0x01 .compare.end_303: pop eax cmp eax, 0x01 je .then_304 jmp .else_304 .then_304: mov eax, [ebp-4] push eax push 0x00000001 pop ebx pop eax add eax, ebx push eax pop eax mov [ebp-4], eax jmp .loop.head_297 jmp .endif_304 .else_304: .endif_304: .loop.end_297: mov esp, ebp pop ebp ret

I have a doubt, here the jump and label .endif_304 is useless? is it ok? @Shaikh-Ubaid

@Shaikh-Ubaid, were you able to understand or get the point?

I have a doubt, here the jump and label .endif_304 is useless? is it ok? @Shaikh-Ubaid

I think for the moment that should be fine.

@Shaikh-Ubaid, were you able to understand or get the point?

Yup, thank you so much for the explanation, Thirumalai.

It seems this condition (if_unique_id.size() - loop_unique_id.size() == label_index - 1) might not be perfect to determine to continue or end the loop. Consider the following example where we have a loop inside an if construct:

def main0(): x: i32 x = (2+3)*5 j: i32 = 0 if True: while x > 0: j += 2 x -= 1 print(x) print(j) main0()

Output:

(lp) ubaid@ubaid-Lenovo-ideapad-330-15ARR:~/OpenSource/lpython$ lpython examples/expr2.py 0 50 (lp) ubaid@ubaid-Lenovo-ideapad-330-15ARR:~/OpenSource/lpython$ lpython examples/expr2.py --backend wasm_x86 -o loop (lp) ubaid@ubaid-Lenovo-ideapad-330-15ARR:~/OpenSource/lpython$ ./loop 24 2 (lp) ubaid@ubaid-Lenovo-ideapad-330-15ARR:~/OpenSource/lpython$

The loop in this case just executes once and then exits. This is because in this case (when visit_Br() is visited) the if count would be 2 and loop count would be 1. The label_index would be 1. The condition would be 2 - 1 == 1 - 1, that is 1 == 0, which is not true and hence the loop exits.

I think we might need to simulate how wasm works in x86. We might need to use same label indexes (or same label index space) for if's and loop's (just like wasm does).

It seems this condition (if_unique_id.size() - loop_unique_id.size() == label_index - 1) might not be perfect to determine to continue or end the loop.

+1; I agree with this.
Also, I didn't test the loop within the if statement.
There you go, you've found the actual bug in the loop statement.
Thank you for testing this. I will make this a new issue. Let's work on this and fix it.

ubaidsk · 2022-11-13T20:48:06Z

I wonder if we need the changes (replacing print statements with assert) in the file integration_tests/if_01.py. Previously, this if test case just had print statements. This was useful to us in the way that if we add a new backend (in the future) and if we add support for if-else in this new backend (with no support for exit code yet), we could easily enable this integration_tests/if_01.py. Now, with assert being used in this test case we would either need to write a new simple if-else test case or also add support for assert before enabling if-else test case.

It seems we write new backends less frequently, hence the above seems to be more of a discussion topic/point. I would like to request your views/suggestions on it.

ubaidsk

This is an Amazing PR, Thirumalai! Great Work! Thank you so much for this.

Thirumalai-Shaktivel · 2022-11-14T12:04:26Z

While we were reviewing the if_stmt PR, @certik, said to replace the print with an assert statement. An assert statement would be the best approach to catch the error easily (in the CI). So, I replaced it.

ubaidsk · 2022-11-14T12:16:30Z

While we were reviewing the PR, @certik, said to replace the print with an assert statement. An assert statement would be the best approach to catch the error easily (in the CI). So, I replaced it.

Yup, got it.

This was useful to us in the way that if we add a new backend (in the future) and if we add support for if-else in this new backend (with no support for exit code yet), we could easily enable this integration_tests/if_01.py.

This is more about for future backends. Nothing to worry about at this point. When we add new backends, we would like to enable as many integration_tests as possible. In lpython, many (or most) of the lpython integration test files test multiple features in the same test. Thus, when we add a small feature (since, we would be building a backend incrementally), we are needing to add a new test case which just tests that particular feature. We are unable to enable other tests testing this feature because those tests also test some other feature for which we do not have support yet. I experienced this issue when adding the wasm backend and the wasm_x86 backend. I would like to request everyone's views on this.

Thirumalai-Shaktivel · 2022-11-14T12:41:18Z

Exactly, I experienced the same issue, while implementing this PR. The problem was that in the integration_tests there is a file named loop_01.py. It had both integers and strings to iterate in the loop. As wasm_x86 doesn't support StringLen, I was not able to use it. I created a new file and added all the tests there.

Also, why don't we have more simpler tests?

I think all the if, loop, ... are being tested more robustly in LFortran.
Now, there are two options: either we add more tests in LPython or submit the PR for new backends to LFortran.

Thirumalai-Shaktivel added 2 commits November 12, 2022 12:40

[WASM_x86] Add description to if and compare visitors

1c79a6b

[WASM_x86] Implement Loop statement

580c1f3

Thirumalai-Shaktivel added the wasm_x86 label Nov 12, 2022

Thirumalai-Shaktivel requested a review from ubaidsk November 12, 2022 07:30

Thirumalai-Shaktivel commented Nov 12, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the loop_stmt branch from d9e24a7 to a7ea5e5 Compare November 12, 2022 08:00

Thirumalai-Shaktivel added 3 commits November 12, 2022 17:12

[WASM_x86] Use different variable to store loop_unique_id

bc64ef4

[WASM_x86] Implement Break and Continue statements

5f38adc

Add loop tests and enable wasm_x86 tests in CMakeLists

b7faf79

Thirumalai-Shaktivel force-pushed the loop_stmt branch from a7ea5e5 to b7faf79 Compare November 12, 2022 11:44

Thirumalai-Shaktivel marked this pull request as ready for review November 12, 2022 11:48

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

Thirumalai-Shaktivel mentioned this pull request Nov 12, 2022

WASM to X86 Backend #1222

Closed

certik approved these changes Nov 13, 2022

View reviewed changes

certik merged commit b67b871 into lcompilers:main Nov 13, 2022

Thirumalai-Shaktivel deleted the loop_stmt branch November 13, 2022 05:40

ubaidsk reviewed Nov 13, 2022

View reviewed changes

Thirumalai-Shaktivel mentioned this pull request Nov 14, 2022

[WASM_x86] Bug in the loop statement #1298

Closed

ubaidsk mentioned this pull request Nov 15, 2022

[WASM_x86] Fix a bug that calculates the label_index in the Br visitor #1299

Merged

	// From WebAssembly Docs:
	// Unlike with other index spaces, indexing of labels is relative by
	// nesting depth, that is, label 0 refers to the innermost structured
	// control instruction enclosing the referring branch instruction, while
	// increasing indices refer to those farther out.

Implement Loop statement #1290

Implement Loop statement #1290

Uh oh!

Conversation

Thirumalai-Shaktivel commented Nov 12, 2022

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel commented Nov 13, 2022

Uh oh!

ubaidsk commented Nov 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

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

ubaidsk commented Nov 13, 2022

Uh oh!

ubaidsk left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Thirumalai-Shaktivel Nov 12, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 14, 2022 •

edited

Loading

ubaidsk commented Nov 14, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Nov 14, 2022 •

edited

Loading