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

feat: Introduce and implement IntrinsicImpureSubroutine #3650

Merged
merged 12 commits into from
Mar 16, 2024

Conversation

Pranavchiku
Copy link
Contributor

@Pranavchiku Pranavchiku commented Mar 14, 2024

Closes #2591

ASR Definition

| IntrinsicImpureSubroutine(int intrinsic_id, expr* args, int overload_id)

@Pranavchiku Pranavchiku added asr ASR Related changes asr pass Issue or pull request specific to ASR pass feature Issue or pull request for adding a new feature intrinsic Issue or pull request specific to intrinsic function labels Mar 14, 2024
@Pranavchiku Pranavchiku marked this pull request as ready for review March 14, 2024 08:40
@@ -57,6 +57,7 @@ stmt
| Stop(expr? code)
| Assert(expr test, expr? msg)
| SubroutineCall(symbol name, symbol? original_name, call_arg* args, expr? dt)
| IntrinsicElementalSubroutine(int intrinsic_id, expr* args, int overload_id)
Copy link
Contributor

@certik certik Mar 14, 2024

Choose a reason for hiding this comment

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

In ASR, IntrinsicElemental must be side-effects-free and deterministic. The random number is neither side-effects-free nor deterministic. The only Fortran intrinsic that would fit into IntrinsicElementalSubroutine is mvbits. However, mvbits can also be implemented as a function in ASR:

CALL MVBITS(FROM, FROMPOS, LEN, TO, TOPOS)

Gets represented as:

TO = MVBITS(FROM, FROMPOS, LEN, TO, TOPOS)

Now all arguments to MVBITS function are intent(in), and it can be implemented as IntrinsicElementalFunction.

Let's implement mvbits using IntrinsicElementalFunction and ensure everything works. Once we convince ourselves that there is no issue, then we know we don't need IntrinsicElementalSubroutine.

Regarding a random number, that has to be IntrinsicImpureSubroutine, per #1658

@Pranavchiku Pranavchiku changed the title feat: Introduce and implement IntrinsicElementalSubroutine feat: Introduce and implement IntrinsicImpureSubroutine Mar 15, 2024
@Pranavchiku Pranavchiku mentioned this pull request Mar 15, 2024
@Pranavchiku
Copy link
Contributor Author

This PR is ready to review.

@certik
Copy link
Contributor

certik commented Mar 16, 2024

Question: does this work when we pass an array to random_number?

@certik certik merged commit 666aa92 into lfortran:main Mar 16, 2024
21 checks passed
@Pranavchiku
Copy link
Contributor Author

Yes it does.

@certik
Copy link
Contributor

certik commented Mar 16, 2024

Ok, it works for an array: https://github.com/lfortran/lfortran/pull/3650/files#diff-e452e55b5ec5613929ce4d9cde98f54efe0d931da016ea716fff121841b69b79R84, specifically, it works for a 1D array. Can you now please add tests for a 2D and 3D array and fix it up to work for them too (if it doesn't)?

The PR looks good, so I merged it. The 2D and 3D array extension can be done in a subsequent PR, I think the design does not close the door to it.

@Pranavchiku
Copy link
Contributor Author

It does work for array of every dimension.

@Pranavchiku
Copy link
Contributor Author

Pranavchiku commented Mar 16, 2024

https://github.com/lfortran/lfortran/pull/3650/files#diff-c00bf96f361ecbfafda30ce2ddfedf0bd47d741ee3e432f0e395d6fbdc816929R588 takes care by creating do loops recursively.

program main
real :: x(5, 6, 7, 8)
call random_number(x)
print *, x
end program
% lfortran a.f90 
7.82636926e-06 1.31537795e-01 7.55605340e-01 4.58650142e-01 5.32767236e-01 2.18959183e-01 4.70446162e-02 6.78864717e-01 
6.79296434e-01 9.34692919e-01 3.83502066e-01 5.19416392e-01 8.30965340e-01 3.45721096e-02 5.34616336e-02 5.29700220e-01 
6.71149373e-01 7.69818621e-03 3.83415639e-01 6.68422356e-02 4.17485982e-01 6.86772704e-01 5.88976622e-01 9.30436492e-01 
8.46166909e-01 5.26928782e-01 9.19648930e-02 6.53918982e-01 4.15999353e-01 7.01190591e-01 9.10320818e-01 7.62198031e-01 
2.62452990e-01 4.74645123e-02 7.36081898e-01 3.28234226e-01 6.32638574e-01 7.56410480e-01 9.91037369e-01 3.65338683e-01 
2.47038886e-01 9.82550263e-01 7.22660422e-01 7.53355861e-01 6.51518583e-01 7.26858824e-02 6.31634712e-01 8.84707153e-01 
2.72709966e-01 4.36411411e-01 7.66494751e-01 4.77731764e-01 2.37774432e-01 2.74906844e-01 3.59264970e-01 1.66507199e-01 
.........

@certik
Copy link
Contributor

certik commented Mar 16, 2024

Very good! Can you add a test for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asr pass Issue or pull request specific to ASR pass asr ASR Related changes feature Issue or pull request for adding a new feature intrinsic Issue or pull request specific to intrinsic function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement intrinsic random_number
2 participants