Page MenuHomePhabricator

[flang] Checks for pointers to intrinsic functions
ClosedPublic

Authored by ekieri on Oct 24 2021, 9:41 AM.

Details

Summary

Check that when a procedure pointer is initialised or assigned with an intrinsic
function, or when its interface is being defined by one, that intrinsic function
is unrestricted specific (listed in Table 16.2 of F'2018).

Mark intrinsics LGE, LGT, LLE, and LLT as restricted specific. Getting their
classifications right helps in designing the tests.

Diff Detail

Event Timeline

ekieri created this revision.Oct 24 2021, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2021, 9:41 AM
ekieri requested review of this revision.Oct 24 2021, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2021, 9:41 AM
PeteSteinfeld accepted this revision.Oct 24 2021, 12:39 PM

Looks great!

Thanks for doing this.

flang/test/Semantics/resolve46.f90
2–4

Please add a comment to note that this test also tests for constraint C1515.

This revision is now accepted and ready to land.Oct 24 2021, 12:39 PM
ekieri updated this revision to Diff 382006.Oct 25 2021, 8:16 AM

Address comment from Pete

Thanks @PeteSteinfeld for the review! I also added a comment for C1519 (initialisation of procedure pointers). All aspects of those constraints are not tested, but I intend to continue working towards that in subsequent patches.

ekieri marked an inline comment as done.Oct 25 2021, 8:20 AM
klausler accepted this revision.Oct 25 2021, 9:47 AM
klausler added inline comments.
flang/lib/Evaluate/characteristics.cpp
421–426

Possibly easier to follow:

auto intrinsic{...};
if (intrinsic && intrinsic->isRestrictedSpecific) {
  intrinsic.reset(); // exclude intrinsics from table 16.3
}
return intrinsic.
flang/lib/Semantics/check-declarations.cpp
636

You could emit distinct messages for the two possible error cases (!intrinsic for one, intrinsic->isRestrictedSpecific for the other).

If you choose to emit just the one message, the predicate could be made easier to follow as:

!intrinsic || intrinsic->isRestrictedSpecific

to avoid making the reader of the code have to figure out multiple negations.

785

My comments from line ca. 630 above also apply here.

ekieri updated this revision to Diff 382090.Oct 25 2021, 12:51 PM

Address comments from Peter

ekieri marked 3 inline comments as done.Oct 25 2021, 1:00 PM

Thanks @klausler for your comments!

I do not have commit access. If this looks ok to you, could you land the patch for me? Please use “Emil Kieri <j.emil.kieri@gmail.com>” to commit it.

Cheers, Emil

flang/lib/Evaluate/characteristics.cpp
421–426

Yes, thanks!

flang/lib/Semantics/check-declarations.cpp
636

I would prefer to stay with just the one message. I think that better matches the actual condition (must be listed in 16.2), handling the cases separately would imho follow the implementation details rather than the underlying logic.

Thanks for the tip with the predicate! Looks cleaner, double negations should clearly be avoided.

Rather than Peter committing this, it’s better for you to do it yourself. Here’s how.

The first step is to get committer access to the llvm-project repository. You can request commit access for the llvm-project here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. Once access is granted, an invitation should be visible here: https://github.com/llvm.

Once you have committer access (actually, you can do this now), you should incorporate your changes into the latest source code from llvm-project, and make sure that everything builds and tests correctly. Here’s how to do that:

  1. Start in your private repository in the branch that contains your changes.
  2. If you have multiple commits, run git rebase -i to squash them into a single commit.
  3. Merge the latest changes from llvm-project into your branch:

• git checkout main
• git pull
• git checkout mybranch
• git rebase main

  1. Rebuild and retest to verify that your changes still work.
  2. Push your changes to the main branch in the llvm-repository: git push origin mybranch:main
ekieri marked 2 inline comments as done.Oct 25 2021, 1:43 PM

Am I not supposed to havet a track record within the project to get commit access? The policy you linked suggests that. This is my first contribution to llvm.

Am I not supposed to havet a track record within the project to get commit access? The policy you linked suggests that. This is my first contribution to llvm.

@ekieri, I've discussed this with Peter, and we both think that the best thing is for you to get commit access and do the commit yourself.

Am I not supposed to havet a track record within the project to get commit access? The policy you linked suggests that. This is my first contribution to llvm.

@ekieri, I've discussed this with Peter, and we both think that the best thing is for you to get commit access and do the commit yourself.

Ok, thanks. I have sent a request.

This revision was automatically updated to reflect the committed changes.

That worked out nicely, thanks a lot for all your help!