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

(close #1987) Replace Intrinsic Operators with IntrinsicCalls #2298

Merged
merged 35 commits into from
Sep 19, 2023

Conversation

sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented Aug 30, 2023

This allows to delete lots of code (Nary Operations, named argument operations) and improve other code (use is_pure, is_elemental, is_inquiry) for intrinsics as if they are regular calls instead of having special cases.

This turned out to be a big PR so I stopped here, but I would like to follow up with:
#2102 Which may mean moving the intrinsic enum from IntrinsicCall to a new IntrinsicSymbol
#2302 To also list the mandatory intrinsic arguments. (which I could do it this PR if the reviewer is fine with a longer one)

Also closes: #1366, #537, #1161, #1837 and #414 ? and PR #2222

@sergisiso sergisiso self-assigned this Aug 30, 2023
@sergisiso sergisiso temporarily deployed to integration August 30, 2023 10:46 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (b4ceb72) 99.85% compared to head (3a8afd3) 99.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2298      +/-   ##
==========================================
- Coverage   99.85%   99.84%   -0.01%     
==========================================
  Files         342      342              
  Lines       46144    46069      -75     
==========================================
- Hits        46075    45998      -77     
- Misses         69       71       +2     
Files Changed Coverage Δ
src/psyclone/domain/lfric/__init__.py 100.00% <ø> (ø)
src/psyclone/psyir/nodes/__init__.py 100.00% <ø> (ø)
src/psyclone/psyir/nodes/ranges.py 100.00% <ø> (ø)
...clone/psyir/transformations/intrinsics/__init__.py 100.00% <ø> (ø)
...psyir/transformations/intrinsics/sum2code_trans.py 100.00% <ø> (ø)
...ests/psyir/frontend/fparser2_alloc_handler_test.py 100.00% <ø> (ø)
...mmon/transformations/kernel_module_inline_trans.py 100.00% <100.00%> (ø)
...main/gocean/transformations/gocean_opencl_trans.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_builtins.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_loop_bounds.py 100.00% <100.00%> (ø)
... and 46 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergisiso sergisiso temporarily deployed to integration August 31, 2023 11:38 — with GitHub Actions Inactive
@sergisiso sergisiso temporarily deployed to integration September 1, 2023 15:56 — with GitHub Actions Inactive
@arporter
Copy link
Member

The NEMO integration tests (ecmwf build) failed with:

NEMOGCM_V40/cfgs/SPITZ12_openmp_cpu/WORK/obs_fbm.f90(1024): error #6410: This name has not been declared as an array or a function.   [CDLTMP]
      cdltmp(idx) = ACHAR(IACHAR(cdltmp(idx)) + 32)
----------^
NEMOGCM_V40/cfgs/SPITZ12_openmp_cpu/WORK/obs_fbm.f90(1024): warning #7319: This argument's data type is incompatible with this intrinsic procedure; procedure assumed EXTERNAL.   [IACHAR]
      cdltmp(idx) = ACHAR(IACHAR(cdltmp(idx)) + 32)
-------------------------------------^
NEMOGCM_V40/cfgs/SPITZ12_openmp_cpu/WORK/obs_fbm.f90(1024): error #6404: This name does not have a type, and must have an explicit type.   [IACHAR]
      cdltmp(idx) = ACHAR(IACHAR(cdltmp(idx)) + 32)
------------------------------^
NEMOGCM_V40/cfgs/SPITZ12_openmp_cpu/WORK/obs_fbm.f90(1024): error #6303: The assignment operation or the binary expression operation is invalid for the data types of the two operands.   [ACHAR]
      cdltmp(idx) = ACHAR(IACHAR(cdltmp(idx)) + 32)
------------------------^

Please could you take a look.

@sergisiso
Copy link
Collaborator Author

The integration test issue comes from expanding the ranges into a loop for:
cdltmp(1:1) = ACHAR(IACHAR(cdltmp(1 : 1)) + 32)
because it now knows the intrinsics are elemental. But intel compiler is fine with the (1:1) range but not the:

do idx = 1, 1, 1
    cdltmp(idx) = ...

@sergisiso sergisiso temporarily deployed to integration September 18, 2023 11:08 — with GitHub Actions Inactive
@sergisiso
Copy link
Collaborator Author

The issue with NEMO integration test is now fixed, I decided to filter the file and leave the fix to another PR because this one is already quite big and I noticed that the omp_gpu_trans had this file already filtered, so both scripts are now doing this same.
Also note that this PR provides an improvement from 22s to 19.7s to the NEMO OpenACC kernels.

@sergisiso
Copy link
Collaborator Author

@arporter This is ready for another review. I submitted an integration test job that came out green and I have fixed a couple of codecov issues to make the patch 100%.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Well done on fixing things and thanks for improving the code by adding the dangling Assignment node. I just have a couple of small queries about that and then this can be merged.

src/psyclone/psyir/symbols/datatypes.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/datatypes.py Show resolved Hide resolved
@sergisiso
Copy link
Collaborator Author

@arporter This is ready for another review. I have addressed one of the comments and just replied to the other, let me know if you disagree with the second.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All requested changes have been made.
Integration tests were all green (20 hrs ago).
Will proceed to merge.

@arporter arporter merged commit eba697a into master Sep 19, 2023
10 of 11 checks passed
@arporter arporter deleted the 1987_operators_to_intrinsic_calls branch September 19, 2023 07:51
JulienRemy added a commit to JulienRemy/PSyclone that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PSyIR Core PSyIR functionality ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[psyir] Support allocate/deallocate and random_number intrinsics
2 participants