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

[peixin]

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.

[peixin]

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

[kiranchandramohan]

flang/test/Semantics/OpenMP/omp-copyin06.f90

11

Do warnings work with test_errors?

[kiranchandramohan]

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

[peixin]

In D127601#3577825, @kiranchandramohan wrote:

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

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]

In D127601#3577825, @kiranchandramohan wrote:

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.

[kiranchandramohan]

Thanks. LGTM.

[ekieri]

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]

Change FileCheck into WARNING preparing for D125804.

[peixin]

(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.

flang/lib/Semantics/check-omp-structure.cpp

 
 // Following clauses do not have a separate node in parse-tree.h.
 CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
 CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
 CHECK_SIMPLE_CLAUSE(AtomicDefaultMemOrder, OMPC_atomic_default_mem_order)
 CHECK_SIMPLE_CLAUSE(Affinity, OMPC_affinity)
 CHECK_SIMPLE_CLAUSE(Allocate, OMPC_allocate)
 CHECK_SIMPLE_CLAUSE(Capture, OMPC_capture)
-CHECK_SIMPLE_CLAUSE(Copyin, OMPC_copyin)
 CHECK_SIMPLE_CLAUSE(Default, OMPC_default)
 CHECK_SIMPLE_CLAUSE(Depobj, OMPC_depobj)
 CHECK_SIMPLE_CLAUSE(Destroy, OMPC_destroy)
 CHECK_SIMPLE_CLAUSE(Detach, OMPC_detach)
 CHECK_SIMPLE_CLAUSE(DeviceType, OMPC_device_type)
 CHECK_SIMPLE_CLAUSE(DistSchedule, OMPC_dist_schedule)
 CHECK_SIMPLE_CLAUSE(DynamicAllocators, OMPC_dynamic_allocators)
 CHECK_SIMPLE_CLAUSE(Exclusive, OMPC_exclusive)
void OmpStructureChecker::Enter(const parser::OmpClause::Lastprivate &x) {
   dirClauseTriple.emplace(llvm::omp::Directive::OMPD_sections,
       std::make_pair(
           llvm::omp::Directive::OMPD_parallel, llvm::omp::privateReductionSet));
 
   CheckPrivateSymbolsInOuterCxt(
       currSymbols, dirClauseTriple, GetClauseKindForParserClass(x));
 }
 
+void OmpStructureChecker::Enter(const parser::OmpClause::Copyin &x) {
+  CheckAllowed(llvm::omp::Clause::OMPC_copyin);
+  for (const auto &ompObject : x.v.v) {
+    common::visit(
+        common::visitors{
+            [&](const parser::Designator &) {
+              if (const auto *name{parser::Unwrap<parser::Name>(ompObject)}) {
+                if (IsPolymorphicAllocatable(*name->symbol)) {
+                  context_.Say(name->source,
+                      "If a polymorphic variable with allocatable attribute "
+                      "'%s' is in COPYIN clause, the behavior is "
+                      "unspecified"_port_en_US, name->symbol->name());
+                }
+              }
+            },
+            [&](const parser::Name &) {}, // common block
+        },
+        ompObject.u);
+  }
+}
+
 llvm::StringRef OmpStructureChecker::getClauseName(llvm::omp::Clause clause) {
   return llvm::omp::getOpenMPClauseName(clause);
 }
 
 llvm::StringRef OmpStructureChecker::getDirectiveName(
     llvm::omp::Directive directive) {
   return llvm::omp::getOpenMPDirectiveName(directive);
 }

flang/test/Semantics/OpenMP/omp-copyin06.f90

+! RUN: %python %S/../test_errors.py %s %flang -fopenmp
+! OpenMP Version 5.0
+! 2.19.6.1 copyin Clause
+! If the list item is a polymorphic variable with the allocatable attribute,
+! the behavior is unspecified.
+
+subroutine copyin()
+  class(*), allocatable, save :: x
+  !$omp threadprivate(x)
+
+  !WARNING: If a polymorphic variable with allocatable attribute 'x' is in COPYIN clause, the behavior is unspecified
+  !$omp parallel copyin(x)
+    call sub()
+  !$omp end parallel
+
+end
+