Skip to content
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

WASM: Implement WASMAssembler Class #1675

Merged
merged 4 commits into from
Apr 11, 2023

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid commented Apr 7, 2023

fixes lfortran/lfortran#589

Also towards #1485

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft April 7, 2023 07:12
@Shaikh-Ubaid Shaikh-Ubaid changed the title WASM: Initial support for WASMAssembler WASM: Implement WASMAssembler Class Apr 7, 2023
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review April 7, 2023 10:50
@Shaikh-Ubaid Shaikh-Ubaid requested a review from certik April 7, 2023 10:50
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft April 7, 2023 10:51
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review April 7, 2023 11:01
@Shaikh-Ubaid
Copy link
Collaborator Author

This is ready. Please review and share feedback.

@Shaikh-Ubaid
Copy link
Collaborator Author

Shaikh-Ubaid commented Apr 7, 2023

X86Assembler is present in LCompilers namespace (that is outside-of/not-in X86 namespace). Therefore, this commit WASM: Bring WASMAssembler outside of wasm namespace.

@Shaikh-Ubaid
Copy link
Collaborator Author

This PR might have conflicts with the sync PR. I will rebase (and/or resolve any conflicts) after completion/merging of the sync PR.

@czgdp1807 czgdp1807 marked this pull request as draft April 7, 2023 11:06
@czgdp1807
Copy link
Collaborator

This PR might have conflicts with the sync PR. I will rebase (and/or resolve any conflicts) after completion/merging of the sync PR.

Marking it for draft until then.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review April 8, 2023 01:45
@Shaikh-Ubaid
Copy link
Collaborator Author

This is ready. Please review and share feedback.

Vec<uint8_t> m_global_section;
Vec<uint8_t> m_export_section;
Vec<uint8_t> m_code_section;
Vec<uint8_t> m_data_section;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part should go into WASMFormat class. The WASMAssembler should just create the m_code_section part, the rest should be handled by WASMFormat.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The WASMFormat Class is named as WASMAssembler here. The WASMAssembler which just creates the m_code_section is named as WASMInstsAssembler. I will update the names if these are confusing.

Currently, the WASMAssembler class inherits from WASMInstsAssembler thereby obtaining/having all the features/powers of creating the wasm format as well as the wasm instructions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the names are confusing to me. I would keep them completely separate no inheritance. The "assembler" knows how to convert instructions to a binary representation. The "binary format" knows how to create sections, headers, etc. and how to create the final binary. It just copies over the result from the "assembler". If you want, we can discuss this at our meeting more and see what would be a good design.

Copy link
Collaborator Author

@Shaikh-Ubaid Shaikh-Ubaid Apr 9, 2023

Choose a reason for hiding this comment

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

The code section in wasm along with storing instructions, also stores local variables of the function. Also, for every code section defined in m_code_section, there must be a function type (added/referenced from m_type_section) in m_func_section. Thus, it seems wasm instructions are not fully separate from wasm format. Hence, currently, the approach in this PR defines WASMAssembler as a single interface to wasm related operations.

Sure Sir, we can discuss it in the meet. Any approach works for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the above comment #1675 (comment).

@czgdp1807 czgdp1807 marked this pull request as draft April 10, 2023 04:30
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review April 11, 2023 17:10
Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is fine. We have the "intruction assembler" class that is automatically generated and handles just converting instructions to binary, into the code section (only). Then we have the "full assembler" class that is built on top and handles everything else and emits the whole WASM binary format, which contains various sections and other parts that must work well together.

@Shaikh-Ubaid Shaikh-Ubaid merged commit e1c55fd into lcompilers:main Apr 11, 2023
@Shaikh-Ubaid Shaikh-Ubaid deleted the wasm_assembler branch April 11, 2023 18:49
Shaikh-Ubaid added a commit to Shaikh-Ubaid/lfortran that referenced this pull request Apr 12, 2023
There are considerable changes in lcompilers/lpython#1675. After its merging, I think it is better to sync the wasm backend early to avoid potential conflicts in future.
Shaikh-Ubaid added a commit to Shaikh-Ubaid/lfortran that referenced this pull request Apr 12, 2023
There are considerable changes in lcompilers/lpython#1675. After its merging, I think it is better to sync the wasm backend early to avoid potential conflicts in future.
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.

Simplify WASM assembler
3 participants