This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add co_sum to the list of intrinsics and update test
ClosedPublic

Authored by ktras on Nov 17 2021, 7:54 PM.

Details

Summary
Add the collective subroutine, co_sum, to the list of intrinsics.
In accordance with 16.9.50 and 16.9.137, add a check for and an
error if coindexed objects are being passed to certain arguments
in co_sum and in move_alloc. Add a semantics test to check that
this error is successfully caught in calls to move_alloc. Remove
the XFAIL directive, update the ERROR directives and add
standard-conforming and non-standard conforming calls in the
semantics test for co_sum.

Diff Detail

Event Timeline

ktras created this revision.Nov 17 2021, 7:54 PM
ktras requested review of this revision.Nov 17 2021, 7:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 7:54 PM
klausler added inline comments.Nov 18 2021, 8:12 AM
flang/lib/Evaluate/intrinsics.cpp
1081

"A=", "STAT=", and "ERRMSG=" are not allowed to be coindexed objects (16.9.50); how will you enforce and test this restriction?

ktras updated this revision to Diff 432641.May 27 2022, 3:08 PM
ktras retitled this revision from [flang] Add evaluation for the collective intrinsic, co_sum to [flang] Add co_sum to the list of intrinsics and update test.
ktras edited the summary of this revision. (Show Details)
ktras added reviewers: rouson, PeteSteinfeld, jeanPerier.
  • Fix rank of the co_sum dummy argument a in flang/lib/Evaluate/intrinsics.cpp
  • Add check for and error if a coindexed object is passed to dummy arguments a, stat, and errmsg in co_sum, and dummy arguments from, to, stat, and errmsg in move_alloc.
  • Add semantics test to ensure that coindexed error is being caught in calls to move_alloc.
  • Update semantics test for co_sum by fixing ERROR directives for the new error and adding one non-standard conforming call.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 3:08 PM
ktras marked an inline comment as done.May 27 2022, 3:10 PM
ktras added inline comments.
flang/lib/Evaluate/intrinsics.cpp
1081

Thanks for the feedback and sorry that it took me so long to respond.
I have now added a check for coindexed objects for these arguments.

ktras marked an inline comment as done.May 27 2022, 3:32 PM
jeanPerier added inline comments.Jun 8 2022, 1:31 AM
flang/lib/Evaluate/intrinsics.cpp
1649

I think this custom check might better belong in ApplySpecificChecks that does custom checks that do not fit in the automated checks.

ktras planned changes to this revision.Jun 8 2022, 10:07 AM
ktras added inline comments.
flang/lib/Evaluate/intrinsics.cpp
1649

Thanks for pointing that function out. I agree that moving this check to ApplySpecificChecks() seems like an appropriate change. I will work on doing that and updating the diff. Thanks for the feedback!

ktras updated this revision to Diff 443995.Jul 12 2022, 10:03 AM

Move check for coindexed objects for certain arguments in co_sum and move_alloc to a new function CheckForCoindexedObjects, which is called from ApplySpecificChecks for the specific cases of co_sum and move_alloc. Add some calls to the semantics tests to better check this implementation.

ktras marked an inline comment as done.Jul 12 2022, 10:03 AM
jeanPerier accepted this revision.Jul 13 2022, 3:47 AM

LGTM, thanks

This revision is now accepted and ready to land.Jul 13 2022, 3:47 AM
ktras added a comment.Jul 14 2022, 1:49 PM

@jeanPerier Thanks for the review!

ktras reopened this revision.EditedJul 14 2022, 3:48 PM

I pushed this change today and had to revert the commit because it caused some unexpected test failures. The three tests that failed on the buildbots (though these tests didn't consistently fail across the different buildbots) were all flang semantics tests and are:

  • flang/test/Semantics/collective01.f90 - the test that I removed the XFAIL directive from in this differential
  • flang/test/Semantics/move_alloc.f90 - the new test from this differential
  • flang/test/Semantics/doconcurrent01.f90 - a test with move_alloc calls

This reopens the revision.

This revision is now accepted and ready to land.Jul 14 2022, 3:48 PM
ktras updated this revision to Diff 455420.Aug 24 2022, 4:30 PM
ktras edited the summary of this revision. (Show Details)

Updated the new function CheckForCoindexedObjects (and renamed it CheckForCoindexedObject) to take one ActualArgument and to check just that argument for a coindexed object. This fixes the non-deterministic test failure I was finding with my previous implementation, plus it also is a better function for when I add the atomic subroutines and use it to check only one of their arguments.

ktras updated this revision to Diff 456380.Aug 29 2022, 9:33 AM

Forgot to run git-clang-format after one last small change I made, so this updated diff fixes the one line changed by git-clang-format.

ktras updated this revision to Diff 457023.EditedAug 31 2022, 10:58 AM

Remove unnecessary if-stmt in new function, CheckForCoindexedObject.

ktras added a comment.Sep 1 2022, 5:44 PM

@jeanPerier I have updated this differential since you approved it, would you like to check the changes? My new function now just checks one ActualArgument to make sure it is not a coindexed-object, instead of a list of ActualArguments. This fixes some test failures I was getting, and also I think matches better the needs of the other subroutines that will need to call this function, like the atomic subroutines, which all have only one ActualArgument that shall not be coindexed.

This revision was automatically updated to reflect the committed changes.