Skip to content

Handle functions returning arrays/structs in subroutine_from_function pass#2408

Merged
czgdp1807 merged 9 commits intolfortran:mainfrom
czgdp1807:func_ret_arr
Sep 15, 2023
Merged

Handle functions returning arrays/structs in subroutine_from_function pass#2408
czgdp1807 merged 9 commits intolfortran:mainfrom
czgdp1807:func_ret_arr

Conversation

@czgdp1807
Copy link
Member

@czgdp1807 czgdp1807 commented Sep 12, 2023 •
edited
Loading

Corresponding LPython PR - lcompilers/lpython#2325

@czgdp1807 czgdp1807 mentioned this pull request Sep 14, 2023
Open
@czgdp1807 czgdp1807 marked this pull request as ready for review September 14, 2023 16:42
@czgdp1807
Copy link
Member Author

czgdp1807 commented Sep 14, 2023

This is ready for review. However, please do not merge this. I will apply the same changes to LPython and then merge both PRs together.

certik
certik reviewed Sep 14, 2023
View reviewed changes
certik
certik reviewed Sep 14, 2023
View reviewed changes
certik
certik reviewed Sep 14, 2023
View reviewed changes
Copy link
Contributor

@certik certik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also a test needs to be added that fails in main, but works with this PR.

Otherwise it mostly looks good I think. I left some comments.

@certik certik marked this pull request as draft September 14, 2023 18:31
@czgdp1807
Copy link
Member Author

czgdp1807 commented Sep 14, 2023 •
edited
Loading

Also a test needs to be added that fails in main, but works with this PR.

This PR is a refactoring PR. We discussed to do this in our meetings. It doesn't fix anything just decouples functions returning arrays from array_op pass and puts the same in subrouting_from_function pass.

@certik
Copy link
Contributor

certik commented Sep 15, 2023

If it is just refactoring, then it's fine.

@czgdp1807 czgdp1807 marked this pull request as ready for review September 15, 2023 06:43
certik
certik reviewed Sep 15, 2023
View reviewed changes
src/libasr/pass/pass_manager.h
"array_op",
"symbolic",
"intrinsic_function",
"subroutine_from_function",
Copy link
Contributor

@certik certik Sep 15, 2023

Choose a reason for hiding this comment

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

I can see two subroutine_from_function passes here. I assume you need to need to run it after intrinsic_function, since some implementations return an array?

Why do you need to run the first one --- is there a requirement that it must run before array_op?

Copy link
Member Author

@czgdp1807 czgdp1807 Sep 15, 2023

Choose a reason for hiding this comment

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

Yes. We need to convert all the functions returning arrays into subroutines and their function calls before sending to array_op pass where only array operations are dealt with.

Copy link
Contributor

@certik certik Sep 15, 2023

Choose a reason for hiding this comment

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

Why not calling intrinsic_function before array_op?

certik
certik approved these changes Sep 15, 2023
View reviewed changes
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 good enough to merge, so I am going to approve it.

I left a comment.

@czgdp1807
Copy link
Member Author

czgdp1807 commented Sep 15, 2023

Well, LPython has some bugs so I am working on fixing it there. Will send a PR there soon. I am working on it as much as I can. We need this because this has uncovered a lot of bugs.

@czgdp1807 czgdp1807 enabled auto-merge September 15, 2023 17:31
@czgdp1807 czgdp1807 mentioned this pull request Sep 15, 2023
Merged
@czgdp1807 czgdp1807 merged commit 5acdab5 into lfortran:main Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@certik certik certik approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@czgdp1807 @certik