[PATCH] D90697: [Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

The following are not implemented. They also don't seem to be in the list of items to do. Do you know why they were not added?
-> The type and the rank of a list item that appears in a reduction clause must be valid for the combiner and initializer.
-> Additionally, the list item or the pointer component of the list item must not be deallocated, allocated, or pointer assigned within the region.
-> Additionally, the list item or the allocatable component of the list item must be neither deallocated nor allocated within the region.



================
Comment at: flang/lib/Semantics/check-directive-structure.h:137
     std::list<C> actualClauses;
+    std::list<Symbol *> reductionSymbols;
   };
----------------
Should this be a list? Can it be a vector or llvm::SmallVector?
https://llvm.org/docs/ProgrammersManual.html#list


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:512-514
+                  CheckReductionVariableDefinable(*symbol);
+                  CheckReductionVariableIntentIn(*symbol);
+                  CheckMultipleReductionVariable(symbol);
----------------
I believe similar functionality was implemented before. eg CheckIntentInPointer. Can it be reused? If not and if they are simple then consider inlining it here.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:572
+                "A list item that appears in a REDUCTION clause"
+                " cannot have a zero-length array section."_err_en_US,
+                ContextDirectiveAsFortran());
----------------
CheckDependArraySection has a similar check. Can you either reuse or refactor such that common code is not duplicated?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:578
+            auto stride{evaluate::ToInt64(*st)};
+            if ((stride && stride != 1)) {
+              context_.Say(GetContext().clauseSource,
----------------
This will not catch cases like the following.

```
program mn
  integer :: c(10,10,10)
  integer :: i
  integer :: k = 10

  !$omp parallel do reduction(+:c(1:10,5,1:6))
  do i = 1, 10
    k = k + 1
  end do
  !$omp end parallel do
  print *, is_contiguous(c(1:10,5,1:6))
end program
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:581
+                  "A list item that appears in a REDUCTION clause"
+                  " should have a continuous storage array section."_err_en_US,
+                  ContextDirectiveAsFortran());
----------------
continuous -> contiguous?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:639
 }
+void OmpStructureChecker::CheckPrivateClauseRestrictions(
+    const parser::OmpObjectList &objList) {
----------------
This does not seem to work for the following case. I think you should start from the reduction list and the check whether those variables are private.

```
program omp_reduction

  integer :: i
  integer :: k = 10
  !$omp parallel private(k)
  !$omp do reduction(+:k)
  do i = 1, 10
    k = k + 1
  end do
  !$omp end do
  !$omp end parallel
end program
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90697/new/

https://reviews.llvm.org/D90697