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

Expressions: Handle intrinsic function calls #416

Merged
merged 9 commits into from
Nov 21, 2024

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Oct 25, 2024

This PR adds handling of intrinsic function calls by adding an is_intrinsic property to ProcedureType and adding the procedure type to the parsing scope in frontends before creating the respective ProcedureSymbol.

We also add the new is_intrinsic flag to the symbol attributes table for quick lookup in scheduler items, as we never want to chase intrinsics.

EDIT: This also needs the fix from #439 and needs a rebase once that is merged.

@mlange05 mlange05 requested a review from reuterbal October 25, 2024 12:28
Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/416/index.html

@mlange05 mlange05 force-pushed the naml-intrinsic-procedure-type branch from 7c0c12d to be1be5c Compare October 28, 2024 13:30
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.32%. Comparing base (ebc7b69) to head (bd243ac).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #416   +/-   ##
=======================================
  Coverage   93.31%   93.32%           
=======================================
  Files         212      212           
  Lines       40728    40769   +41     
=======================================
+ Hits        38007    38047   +40     
- Misses       2721     2722    +1     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 93.28% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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


🚨 Try these New Features:

@mlange05 mlange05 force-pushed the naml-intrinsic-procedure-type branch from 44613f6 to 0c432a7 Compare November 8, 2024 04:22
@mlange05 mlange05 force-pushed the naml-intrinsic-procedure-type branch 2 times, most recently from 14d8d85 to 43cff05 Compare November 19, 2024 15:32
@mlange05 mlange05 marked this pull request as ready for review November 19, 2024 16:09
@mlange05 mlange05 force-pushed the naml-intrinsic-procedure-type branch from 43cff05 to ed006d5 Compare November 19, 2024 20:47
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great. One thing I'm missing here is the situation when intrinsic functions are hidden because of user-defined functions with the same name. This is unfortunately allowed by the standard and used in practice...

@@ -2547,7 +2547,17 @@ def visit_Function_Reference(self, o, **kwargs):


def visit_Intrinsic_Function_Reference(self, o, **kwargs):

# Register the ProcedureType in the scope before the name lookup
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we somehow verify here that this is indeed the intrinsic function that is used, and not a locally defined function that happens to be called the same as an intrinsic function?

A concrete example is DOT_PRODUCT, where an implementation in IFS shadows the intrinsic Fortran function. In that case, the procedure symbol should likely not be classified as intrinsic.

It would probably be good to have a test that covers such shadowing.

@mlange05 mlange05 force-pushed the naml-intrinsic-procedure-type branch from ed006d5 to d73706c Compare November 21, 2024 07:58
@mlange05
Copy link
Collaborator Author

Hi @reuterbal , apologies, I rebased over main to sanity reasons. However, the response to your comment is in the second to last commit. With the top commit I piggy-backed a mitigation fix for ab upstream version incompatibility with last night's pydantic release. Please let me know if you'd like this to be a separate PR.

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Many thanks, good to know about the limitation with shadowing in OMNI!
Thanks also for the pydantic hotfix.

@reuterbal reuterbal merged commit 72d35fb into main Nov 21, 2024
13 checks passed
@reuterbal reuterbal deleted the naml-intrinsic-procedure-type branch November 21, 2024 09:20
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.

2 participants