Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

Thirumalai-Shaktivel
Copy link
Collaborator

Fixes: #1298

@ubaidsk
Copy link
Collaborator

ubaidsk commented Nov 15, 2022

Thirumailai, it seems the condition (loop_unique_id.size() + if_unique_id.size() - cur_nesting_length == label_index + 1) might not be simple to understand. I think there could be a simpler way if we try to simulate similar to how wasm does. wasm uses the same label index space for if's and loop's. Similary, if we use the same label index space for if's and loop's in wasm->x86 backend, I think we can find a simpler approach. Ideally, we should attempt to follow the label indexing as described here #1290 (comment).

wasm uses the same label index space for if's and loop's

By same label index space, I mean that if ids and loop ids would be counted from the same integer domain. That is, if there is an if with an id = 4 (let's say), there could not be a loop with that same id = 4.

@Thirumalai-Shaktivel
Copy link
Collaborator Author

Thirumalai-Shaktivel commented Nov 15, 2022

(loop_unique_id.size() + if_unique_id.size() - cur_nesting_length) is same as nesting_level - cur_loop_nesting_level, right?
Can you please share where the label index space is used in the wasm? or how it is implemented? (P.S. I didn't get it properly)

@ubaidsk
Copy link
Collaborator

ubaidsk commented Nov 15, 2022

(loop_unique_id.size() + if_unique_id.size() - cur_nesting_length) is same as nesting_level - cur_loop_nesting_level, right?

Yup, got it. I think the condition fine for the moment.

Can you please share where the label index space is used in the wasm? or how it is implemented? (P.S. I didn't get it properly)

It is more abstract/conceptual. In simple words, what I mean by wasm uses the same label index space for if's and loop's is that if's and loop's use the same Id sequence (i.e. they take ids from the same set).

As the tests pass, let's merge this for the moment and we can improve upon it incrementally.

Copy link
Collaborator

@ubaidsk ubaidsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested some cases locally. It seems to work.

@Thirumalai-Shaktivel
Copy link
Collaborator Author

Thirumalai-Shaktivel commented Nov 16, 2022

Before splitting into two id's (if_id, loop_id), I thought to use the same approach as wasm does i.e., using nesting_level. Like push_back both loop_id and if_id into one stack. Later I realized it is difficult to retrieve the correct id's in the nested if_loop statement.
Yup, this (loop_unique_id.size() + if_unique_id.size() - cur_nesting_length) seems to work for many cases. But, if you find any bugs, do let me know or create an issue.
Thanks for the approval. Let's merge this.

@Thirumalai-Shaktivel Thirumalai-Shaktivel merged commit ebf8b44 into lcompilers:main Nov 16, 2022
@Thirumalai-Shaktivel Thirumalai-Shaktivel deleted the i_1298 branch November 16, 2022 02:57
@ubaidsk
Copy link
Collaborator

ubaidsk commented Nov 16, 2022

I thought to use the same approach as wasm does i.e., using nesting_level

Yes, I guessed. Since wasm already uses the nesting level and as we are working on wasm to x86, ideally, we should not be needing to use nesting level again (since it is something already figured out at the wasm level).

ideally, we should not be needing to use nesting level again

I am not sure how much this ideal case is possible, but we need-to/should try/experiment and see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WASM_x86] Bug in the loop statement
2 participants