[flang][OpenMP] Fix semantic check of test case in taskloop simd construct

[peixin]

Summary

The following semantic check is removed in OpenMP Version 5.0:

Taskloop simd construct restrictions: No reduction clause can be specified.

Also fix several typos.

[peixin]

Add ! REQUIRES: shell in omp-taskloop-simd01.f90.

[kiranchandramohan]

Thanks, @peixin for this patch. Also great to see you here.

The recommendation is to provide the full context when submitting patches. I think if you use arcanist, this happens by default. If you use the web interface then there are some recommended commands to use to get the full context. Please see the following links.
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Also, could you point me to the place in the standard where this restriction is specified?

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

1035

I think would be better to say that reduction is not expected for taskloop simd? Or is this a common pattern that we have in semantic errors?

I think we capitalize the name of constructs when we report errors. Please check other errors and capitalize if needed.

[peixin]

@kiranchandramohan Thanks for the guide to submit patches and the comments.

The standard is at page 92 in openmp-4.5 specification, which I quote:

Restrictions
The restrictions for the taskloop and simd constructs apply.
No reduction clause can be specified.

I argee that saying the clause is not allowed in the directive is better. Changed the code fix and test case error info. It is common semantic check in DirectiveStructureChecker in flang/lib/Semantics/check-directive-structure.h, which I quote in the following:

if (!GetContext().allowedClauses.test(clause) &&
    !GetContext().allowedOnceClauses.test(clause) &&
    !GetContext().allowedExclusiveClauses.test(clause) &&
    !GetContext().requiredClauses.test(clause)) {
  context_.Say(GetContext().clauseSource,
      "%s clause is not allowed on the %s directive"_err_en_US,
      parser::ToUpperCaseLetters(getClauseName(clause).str()),
      parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()));
  return;
}

The reason why it is not checked in DirectiveStructureChecker is the clause OMPC_Reduction is defined inside OMP_TaskLoopSimd in llvm/include/llvm/Frontend/OpenMP/OMP.td.
I also noticed that there is no this restriction in openmp-5.0 specification.

[clementval]

The reason why it is not checked in DirectiveStructureChecker is the clause OMPC_Reduction is defined inside OMP_TaskLoopSimd in llvm/include/llvm/Frontend/OpenMP/OMP.td.
I also noticed that there is no this restriction in openmp-5.0 specification.

I think this is going against the generic directive checker. If the clause is set as allowed in the OMP.td file it should be allowed. Since the file is shared with clang the semantic should be done on version 5.0 or 5.1.

[peixin]

@clementval Yes, I agree. I did not notice that this restriction was deleted in Openmp-5.0 standard when I upload the first version of this patch. Do you think if I should fix the restriction info in omp-taskloop-simd01.f90 and the typos in omp-clause-validity01.f90, and change the commit message by stating that this restriction is deleted in openmp version 5.0?

[kiranchandramohan]

In D105874#2874181, @peixin wrote:

@clementval Yes, I agree. I did not notice that this restriction was deleted in Openmp-5.0 standard when I upload the first version of this patch. Do you think if I should fix the restriction info in omp-taskloop-simd01.f90 and the typos in omp-clause-validity01.f90, and change the commit message by stating that this restriction is deleted in openmp version 5.0?

Yes, please do.

[peixin]

Thanks for the comments. Update the following:

1. Remove the semantic check of taskloop simd reduction in check-omp-structrue.cpp.
2. Fix the test case of omp-taskloop-simd01.f90 according to OpenMP 5.0 specification.
3. Fix the title and summary.

[kiranchandramohan]

LGTM.

[kiranchandramohan]

Let me know if you don't have commit access and would need help to land this patch.

[arnamoy10]

I will land this patch for him @kiranchandramohan , thanks for reviewing.

[peixin]

Thanks for the help. @kiranchandramohan @arnamoy10

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

 
 // Clauses
 // Mainly categorized as
 // 1. Checks on 'OmpClauseList' from 'parse-tree.h'.
 // 2. Checks on clauses which fall under 'struct OmpClause' from parse-tree.h.
 // 3. Checks on clauses which are not in 'struct OmpClause' from parse-tree.h.
 
 void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
-  // 2.7 Loop Construct Restriction
+  // 2.7.1 Loop Construct Restriction
   if (llvm::omp::doSet.test(GetContext().directive)) {
     if (auto *clause{FindClause(llvm::omp::Clause::OMPC_schedule)}) {
       // only one schedule clause is allowed
       const auto &schedClause{std::get<parser::OmpClause::Schedule>(clause->u)};
       if (ScheduleModifierHasType(schedClause.v,
               parser::OmpScheduleModifierType::ModType::Nonmonotonic)) {
         if (FindClause(llvm::omp::Clause::OMPC_ordered)) {
           context_.Say(clause->source,
if (GetContext().directive == llvm::omp::Directive::OMPD_end_single) {
     CheckNotAllowedIfClause(
         llvm::omp::Clause::OMPC_copyprivate, {llvm::omp::Clause::OMPC_nowait});
   }
 
   CheckRequireAtLeastOneOf();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause &x) {
   SetContextClause(x);
 }
 
 // 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)

flang/test/Semantics/omp-clause-validity01.f90

 ! RUN: %S/test_errors.sh %s %t %flang_fc1 -fopenmp
 ! REQUIRES: shell
 use omp_lib
 ! Check OpenMP clause validity for the following directives:
 !
 !    2.5 PARALLEL construct
 !    2.7.1 Loop construct
 !    ...
 
-! TODO: all the internal errors
-
   integer :: b = 128
   integer :: z, c = 32
   integer, parameter :: num = 16
   real(8) :: arrayA(256), arrayB(512)
 
   integer(omp_memspace_handle_kind) :: xy_memspace = omp_default_mem_space
   type(omp_alloctrait) :: xy_traits(1) = [omp_alloctrait(omp_atk_alignment,64)]
   integer(omp_allocator_handle_kind) :: xy_alloc
!                        allocate-clause
   end do
   !$omp end target
 
   !ERROR: ALLOCATE clause is not allowed on the TARGET DATA directive
   !$omp target data map(from: b) allocate(b)
   do i = 1, N
      z = 2
   enddo
-   !$omp end target data
 
   !ERROR: SCHEDULE clause is not allowed on the PARALLEL directive
   !$omp parallel schedule(static)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end parallel
 
outer: do i=0, 10
     inner: do j=1, 10
       exit
       exit outer
       !ERROR: EXIT to construct 'outofparallel' outside of PARALLEL construct is not allowed
       !ERROR: EXIT to construct 'outofparallel' outside of DO construct is not allowed
       exit outofparallel
     end do inner
   end do outer
-  !$end omp do
+  !$omp end do
   !$omp end parallel
   end do outofparallel
 
 ! 2.7.1  do-clause -> private-clause |
 !                     firstprivate-clause |
 !                     lastprivate-clause |
 !                     linear-clause |
 !                     reduction-clause |

flang/test/Semantics/omp-taskloop-simd01.f90

 ! RUN: %S/test_errors.sh %s %t %flang -fopenmp
-! XFAIL: *
+! REQUIRES: shell
 
-! OpenMP Version 4.5
-! 2.9.3 taskloop simd Construct
-! No reduction clause may be specified for !$omp taskloop simd.
+! OpenMP Version 5.0
+! 2.10.3 taskloop simd Construct
 
 program omp_taskloop_simd
   integer i , j , k
 
-  !ERROR: Unexpected clause specified for !$OMP taskloop simd
   !$omp taskloop simd reduction(+:k)
   do i=1,10000
     do j=1,i
-      call loop_body(i, j)
       k = k + 1
     end do
   end do
   !$omp end taskloop simd
 
 end program omp_taskloop_simd