This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add one semantic check for data-sharing clauses
ClosedPublic

Authored by peixin on Jun 12 2022, 8:22 AM.

Details

Summary

As OpenMP 5.0, for firstprivate, lastprivate, copyin, and copyprivate
clauses, if the list item is a polymorphic variable with the allocatable
attribute, the behavior is unspecified.

Diff Detail

Event Timeline

peixin created this revision.Jun 12 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
peixin requested review of this revision.Jun 12 2022, 8:22 AM

BTW, this is the only missed semantic check for copyin clause.

flang/test/Semantics/OpenMP/omp-copyin06.f90
11 ↗(On Diff #436226)

Do warnings work with test_errors?

This test is applicable for lastprivate, firstprivate, and copyprivate as well. Could you check and add for these as well?

This test is applicable for lastprivate, firstprivate, and copyprivate as well. Could you check and add for these as well?

Sure. Will take a look at them.

flang/test/Semantics/OpenMP/omp-copyin06.f90
11 ↗(On Diff #436226)

Not yet. It is ongoing work in https://reviews.llvm.org/D125804. There are a number of test cases with WARNINGs, so maybe no need to wait for it to be merged?

peixin updated this revision to Diff 436691.Jun 14 2022, 1:19 AM
peixin retitled this revision from [flang][OpenMP] Add one semantic check for copyin clause to [flang][OpenMP] Add one semantic check for data-sharing clauses.
peixin edited the summary of this revision. (Show Details)

This test is applicable for lastprivate, firstprivate, and copyprivate as well. Could you check and add for these as well?

Confirmed and added the checks for other clauses as well.

There are some test cases using FileCheck for warnings. Use FileCheck for warnings.

Thanks. LGTM.

This revision is now accepted and ready to land.Jun 14 2022, 2:15 AM
ekieri added a subscriber: ekieri.Jun 14 2022, 9:56 AM

I think you can use the WARNING-directives without waiting for D125804. It will take me some time to land it, and I do not think it should block other developments. In the meantime, it would only be helpful if WARNING-directives are included in new (and existing) tests, like you did here. It would, for now, give a false impression of the warnings being tested, but at least they will be in the future (and if it regresses before then, I will notice). Refraining from testing warnings in the meantime comes with the risk of forgetting to add the tests later.

(This reasoning applies to D125891 too, and I would by the same arguments propose that you commit (after review) the test case you made for D125371 as well.)

Using FileCheck is of course an option, but the test you provided does not fail on unexpected warnings. In the RFC for D125804, it was generally agreed this is important. This, I learned just now, can be achieved with FileCheck using --implicit-check-not (cf. D126291), but check_errors.py will (when D125804 is landed) be both simpler to use and more rigorous (as it also checks that the warning is caused by the expected line). I do not object to using FileCheck (in general, I think the common infra should be preferred), but if you choose to do so, please use -- implicit-check-not for both warning and portability.

peixin updated this revision to Diff 436989.Jun 14 2022, 6:10 PM

Change FileCheck into WARNING preparing for D125804.

(This reasoning applies to D125891 too, and I would by the same arguments propose that you commit (after review) the test case you made for D125371 as well.)

I am not aware of your patch at that time. Sorry for the inconvenience. Will add the test later.

check_errors.py will (when D125804 is landed) be both simpler to use and more rigorous (as it also checks that the warning is caused by the expected line).

Agree. I will use FileCheck for review since it is easy for reviewers to ensure the warnings are generated correctly. After accepted, I will change the FileCheck into WARNINGs to ease the burden of preparing for your patch.