This is an archive of the discontinued LLVM Phabricator instance.

[flang] Make 'num_images()' intrinsic
ClosedPublic

Authored by ktras on Jul 3 2020, 11:58 AM.

Details

Summary

I added 'num_images()' to the list of functions that are evaluated as intrinsic. I also added a test file in flang/test/Semantics to test calls to 'num_images()'. There was a call to 'num_images()' in flang/test/Semantics/call10.f90 that expected an error, now it no longer produces an error. So I edited that file accordingly. I also edited the intrinsics unit test to add further testing of 'num_images()'.

Diff Detail

Event Timeline

ktras created this revision.Jul 3 2020, 11:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
sscalpone accepted this revision.Jul 3 2020, 12:27 PM

Please remove num_images from the list of unimplemented intrinsics in flang/documentation/Intrinsics.md.

Looks good. Thanks!

flang/test/Semantics/call10.f90
188

I don't think you need a comment because you noted the reason in the commit message.

This revision is now accepted and ready to land.Jul 3 2020, 12:27 PM
klausler requested changes to this revision.Jul 3 2020, 12:35 PM
klausler added inline comments.
flang/lib/Evaluate/intrinsics.cpp
584

num_images is not an intrinsic procedure in subclause 16 of the standard; INTRINSIC NUM_IMAGES would not be a valid statement. It shouldn't be here with the actual intrinsics unless you're proposing to make it one as an extension.

This revision now requires changes to proceed.Jul 3 2020, 12:35 PM
klausler accepted this revision.Jul 3 2020, 12:59 PM

LGTM, and thanks for the contribution.

flang/lib/Evaluate/intrinsics.cpp
584

Whoops, my mistake; I had it confused with another procedure.

This revision is now accepted and ready to land.Jul 3 2020, 12:59 PM
ktras updated this revision to Diff 275557.Jul 5 2020, 11:47 AM

Based on feedback, I have removed 'num_images()' from the to do list in the documentation intrinsics.md. I have also updated the test call10.f90 to remove an unnecessary comment.

ktras marked 3 inline comments as done.Jul 5 2020, 11:48 AM
tskeith added inline comments.Jul 6 2020, 8:59 AM
flang/test/Semantics/num_images.f90
15

Why is the error "too many actual arguments" rather than incorrect type?

23

Similar question here: team_number isn't an unknown keyword argument. The value has the wrong type.

Are these bad error messages found with other intrinsics or unique to `num_images?

ktras marked 2 inline comments as done.Jul 6 2020, 11:28 AM
ktras added inline comments.
flang/test/Semantics/num_images.f90
15

I believe it is because 'num_images()' is overloaded with 3 variants and one of these has no arguments. If an argument is found that doesn't fully match the versions of 'num_images()' that do have arguments, then it seems to be interpreting those incorrect calls as an call to the version with no arguments. Thus the error being "too many actual arguments" if the argument is of an unexpected type or "unknown keyword argument" if a correct keyword argument is used, but with an incorrect type. I haven't looked into if there is a way to change the logic of the errors being produced in these cases.

23

Replied in comment above.

@ktras, do you have the necessary permissions to commit this or do you need someone to do it for you?

ktras added a comment.Jul 7 2020, 11:19 AM

@ktras, do you have the necessary permissions to commit this or do you need someone to do it for you?

No, I do not, so I would need someone to commit it for me if everything looks ready to go. Thank you all for your help!

This revision was automatically updated to reflect the committed changes.

@ktras, are you planning to implement the other coarray intrinsic functions?

ktras added a comment.Jul 7 2020, 5:25 PM

@ktras, are you planning to implement the other coarray intrinsic functions?

Yes, that is my plan. I am looking at 'this_image()' next.

@ktras, are you planning to implement the other coarray intrinsic functions?

Yes, that is my plan. I am looking at 'this_image()' next.

Excellent! I'm working on some NAG tests that use coarrays, and they use this_image().

Thanks for doing this!