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

Pass: Passing Array descriptor by value #77

Closed

Conversation

Shaikh-Ubaid
Copy link
Member

This is a WIP (Work in progress). The tests might not pass.

@Shaikh-Ubaid Shaikh-Ubaid changed the title Pass: Pass Array descriptor by value Pass: Passing Array descriptor by value Aug 3, 2022
@Shaikh-Ubaid
Copy link
Member Author

As suggested in this #70 (comment), using this pass, we need to change

subroutine f(a, b)
integer :: a(:,:)
real :: b(:,:,:)
end subroutine

into

subroutine f(na1, na2, a, nb1, nb2, nb3, b)
integer, intent(in) :: na1, na2, nb1, nb2, nb3
integer :: a(na1, na2)
real :: b(nb1, nb2, nb3)
end subroutine

doubt: Please, could someone possibly share how the following c/cpp code is expected to change when this pass is applied.

void my_func(int **a, int ***b) {
    ... // some statements here
}

class ArrDimsPropagate : public ASR::StatementsFirstBaseWalkVisitor<ArrDimsPropagate>
class ArrDimsPropagate : public ASR::StatementWalkVisitor<ArrDimsPropagate>
Copy link
Member Author

Choose a reason for hiding this comment

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

On line 21, do we need to use PassUtils::PassVisitor or is ASR::StatementWalkVisitor also fine/works?

Comment on lines 50 to 52
ASR::FunctionCall_t xx = const_cast<ASR::FunctionCall_t &>(x);
xx.n_args = new_args.size();
xx.m_args = new_args.p;
Copy link
Member Author

Choose a reason for hiding this comment

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

(Lines 50 to 52) Since, the arguments passed to visit_FunctionCall() (and others) are const, it was no possible to update them. So, I used a const_cast<> to allow us to update values but this leads to warnings as follows during build:

/home/ubaid/OpenSource/lfortran/src/libasr/pass/arr_dims_propagate.cpp:50:29: warning: variable ‘xx’ set but not used [-Wunused-but-set-variable]
   50 |         ASR::FunctionCall_t xx = const_cast<ASR::FunctionCall_t &>(x);

Copy link
Contributor

Choose a reason for hiding this comment

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

The solution here is to have a visitor that allows to modify the ASR tree in-place. We just have to do it.

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 4, 2022

This segfaults currently when printing ASR (using pickle(asr, true /* use colors */, true /* indent */, true /* with_intrinsic_modules */))

@Shaikh-Ubaid
Copy link
Member Author

Shaikh-Ubaid commented Aug 4, 2022

As FunctionCall is an expression, SubroutineCall is a statement and Function and Subroutines are symbols, it seems as if we need/might-need several visitors/ASR-visits, one each for replace/modify expression, replace/modify statement, replace/modify symbol. I think I need help in this, please.

@certik
Copy link
Contributor

certik commented Aug 4, 2022

Creating the general ASR pass might be more complicated, so let's do that later.

For now, just support the same kind of arrays that @czgdp1807 implemented in LFortran to represent with just a pointer.

We will later add an ASR pass to be able to transform more general arrays into that.

Conclusion: just get a(n) style arrays working, and do not support a(:) arrays for now.

@czgdp1807
Copy link
Member

czgdp1807 commented Aug 4, 2022

Creating the general ASR pass might be more complicated, so let's do that later.

Correct me if I am wrong. So we need to do the following things,

  1. Replace procedure signature of the kind f(array1, array2) with f(array1, dim1, dim2, array2, dim3, dim4). Just an example. Programatically this can be generalised to any number of dimensions.
  2. And then replace the declarations inside the procedure for these arrays.
  3. Replace the the procedure calls according to the updated signature.

If so, then I think it shouldn’t be hard but just lengthy. Can I try to do this? In any case if successful then good otherwise we will know the limitations of this strategy.

Please let me know. I will continue this if you agree.

@certik
Copy link
Contributor

certik commented Aug 4, 2022

@czgdp1807 yes exactly. Yes, go ahead and try to do this!

This would be very useful optional pass, that we can reuse across all backends. For some, like LLVM, it will give an extra performance in some cases. For some other, like WASM, it would be needed to even get some cases working.

@czgdp1807
Copy link
Member

Great. Added this to my priority list. Will start working on this.

@czgdp1807
Copy link
Member

N.B. #87. Its a WIP.

@czgdp1807
Copy link
Member

Closing in favour of #87

@czgdp1807 czgdp1807 closed this Aug 8, 2022
@Shaikh-Ubaid Shaikh-Ubaid deleted the pass_arr_des_by_value branch August 19, 2022 12:16
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