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: Use column-major for arrays #621

Merged
merged 2 commits into from
Aug 18, 2022
Merged

Conversation

certik
Copy link
Contributor

@certik certik commented Aug 18, 2022

No description provided.

certik added a commit to certik/lcompilers_frontend that referenced this pull request Aug 18, 2022
@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

This is a breaking change that requires an update of the code in the gui: lfortran/lcompilers_frontend#29, so we should test both locally first, to ensure everything still works.

@Shaikh-Ubaid
Copy link
Member

I was wondering that since it seems many languages (C++, Python, etc.) follow row-major ordering, please, could you possibly share if it would be possible to support row-major ordering at the ASR level?

@Shaikh-Ubaid
Copy link
Member

I was wondering that since it seems many languages (C++, Python, etc.) follow row-major ordering, please, could you possibly share if it would be possible to support row-major ordering at the ASR level?

So, I guess the AST->ASR part should possibly change/handle the array indexing to follow row-major ordering.

@Shaikh-Ubaid
Copy link
Member

doubt: If the wasm backend stores in col-major format, please, could you possibly share how it could/should support lpython.

certik added a commit to certik/lcompilers_frontend that referenced this pull request Aug 18, 2022
certik added a commit to certik/lcompilers_frontend that referenced this pull request Aug 18, 2022
@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

I tested this locally with the updated lcompilers_frontend (lfortran/lcompilers_frontend#29) and it works.

Regarding ASR, I think it simply stores the array indices from left to right. It is independent of the internal order of the arrays.

Great question how this works with LPython. @czgdp1807 do you know how this currently works with the LLVM backend?

One option is to store the ordering in ASR as "memory_layout = RowMajor | ColumnMajor | ...", and the backend would support both. In LPython and LFortran we can optionally expose this to the source level so that the user can choose. The default would be ColumnMajor for Fortran and RowMajor for LPython.

@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

I implemented both orders in the WASM backend, and one can easily choose with a variable. This makes the code ready to be used if we add the flag into ASR itself. I tested both versions locally, they seem to work.

For now I suggest to use column major by default, since we currently only test WASM with LFortran. Later with LPython, we have to keep the column-major for Fortran, and enable row-major just for LPython.

We should also add some tests for this. But that can be done later. I think it's important to do this breaking change soon, and update the demo code, so that it is conforming to Fortran and works well with other Fortran compilers. After we merge this, we will stay backwards compatible.

Copy link
Member

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

It looks great!

@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

If I merge this, will it update correctly automatically online?

@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented Aug 18, 2022

If I merge this, will it update correctly automatically online?

It seems we might need to first merge the corresponding PR lfortran/lcompilers_frontend#29 and then merge merge this PR soon after it. I think the site dev.lfortran.org might not work or fail in the duration between which one PR is merged and other is yet to be merged.

@Shaikh-Ubaid
Copy link
Member

Please, could we possibly hold this for some more time? I am pushing a change that would allow lcompilers_frontend to fetch the wasm builds from wasm_builds and thus the order of merging of both #621 and lfortran/lcompilers_frontend#29 should/would not matter then.

@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

Yes, just ping me when this is ready to be merged.

@Shaikh-Ubaid
Copy link
Member

Yes, just ping me when this is ready to be merged.

Sure, I will message here when ready.

@Shaikh-Ubaid
Copy link
Member

I am pushing a change that would allow lcompilers_frontend to fetch the wasm builds from wasm_builds and thus the order of merging of both #621 and lfortran/lcompilers_frontend#29 should/would not matter then.

It seems, we currently need to solve lfortran/lcompilers_frontend#31 for the above to work.

@Shaikh-Ubaid
Copy link
Member

Shaikh-Ubaid commented Aug 18, 2022

merging of both #621 and lfortran/lcompilers_frontend#29 should/would not matter then.

It seems even currently the ordering does not matter. Both the PRs bring changes which coordinate with each other (one without the other might fail). So, it seems we need both the PRs to be merged at same/similar times.

@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

I think we can merge LFortran first. Then the frontend, which should update the website and in the process download the latest lfortran.wasm?

@czgdp1807
Copy link
Member

Great question how this works with LPython. @czgdp1807 do you know how this currently works with the LLVM backend?

For now its the same as LFortran (libasr is shared) i.e., Column Major Order (CMO). However I can implement Row Major Order as well and it should be a quick work (AFAICT). The question is how urgently we need Row Major Order (RMO) in LPython. If its not that critical then I will implement it later. In addition, how will the choice between RMO and CMO will be exposed in LPython. If it will be a variable attribute or type annotation then I think parser should be updated first to make that a part of AST, then in AST to ASR transition we can make it a part of ASR and finally LLVM/C/C++ backends will index the array according to the type annotation provided for each array.

@Shaikh-Ubaid
Copy link
Member

I think we can merge LFortran first. Then the frontend, which should update the website and in the process download the latest lfortran.wasm?

Yes, right. It looks like ordering matters currently. It seems ordering might now matter once we have lfortran/lcompilers_frontend#31.

@certik certik marked this pull request as ready for review August 18, 2022 13:43
@certik certik merged commit 4f6dff4 into lfortran:main Aug 18, 2022
@certik certik deleted the wasm_column_major branch August 18, 2022 13:43
@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

@czgdp1807 it's a lower priority. Regarding how to expose in LPython: using the NumPy syntax in array, reshape and other functions, they all have the order argument which accepts C, F (for C or Fortran order).

@czgdp1807
Copy link
Member

I see. Makes sense. I think then C and F should be constant inputs so that they are available at compile time. Not something like array = empty(10, order=a_runtime_string), right?

P.S. I will add it to my list but keep it towards the bottom. Let me know whenever the support for this is needed, I will work on it then.

@certik
Copy link
Contributor Author

certik commented Aug 18, 2022

I opened up an issue for this at lcompilers/lpython#987. This is lower priority.

Yes, I would only do strictly compile time, see the issue.

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.

None yet

3 participants