In D90697#2620677, @kiranchandramohan wrote:

In D90697#2617519, @yhegde wrote:

This patch is updated with a few more test cases with max/min etc in omp-reduction07.f90 and also TODO comments. Hope they are acceptable.

Thanks to @clementval and @kiranchandramohan for accepting this PR and for the review comments. Thanks to @praveen for the kind co operation.

Please go ahead and submit.
Thanks for your help and patience @yhegde, @praveen. Goes without saying, You are both welcome to make contributions to Flang and OpenMP in particular.
Best wishes for your future endeavours.

This patch is updated with a few more test cases with max/min etc in omp-reduction07.f90 and also TODO comments. Hope they are acceptable.

In D90697#2610306, @kiranchandramohan wrote:

LGTM. Thanks for the patience.

Thank you for the review comments. Sure will update the diff with the suggestions.

In D96686#2610323, @kiranchandramohan wrote:

LGTM. Yes, using the symbol is the right way here.
Thanks @yhegde for separating this out into a new patch. Thanks also for you patience and all your work for semantic checks for OpenMP.

Patch updated and rebased to check Threadprivate variables in do loops , across the modules.

Thanks for the suggestion. This patch is rebased and updated with the popping of end_section and other end worksharing clauses fix.

This patch is updated after addressing the review comments.

In D90697#2592760, @kiranchandramohan wrote:

There are a few comments not marked as done. Are these not done or have been just not marked done?

Patch updated and rebased after addressing the reviewer's comments.

It works for the following case.

In D96686#2565173, @yhegde wrote:

@kiranchandramohan , I was working with the following test case you have given ,

module md
integer :: i
!$omp threadprivate(i)
end module

program mn
use md
!$omp do
do i = 1, 10

do j = 1, 10
  print *, "Hello"
end do

end do
!$omp end do
end program
+++++++++++++++++++++++++++

The threadPrivateSymbol will get reset once the subroutine called up and it is set as OmpPredetermined and OmpPrivate and adjusted with in the DO loop region and it seems calling DeclareOrMarkOtherAccessEntity to set the Symbol flag to OmpThreadprivate and I think MakeAssocSymbol called with OmpPrivate to create HostAssocDetails. Then I am getting the symbol from HostAssocDtails and then checking whether it is OmpThreadprivate . Its working and throwing the following err.

./tpmodule.f90:9:4: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10 
   ^

if this is the case , I suppose no where I need to call ClearThreadPrivateSymbols() because all the symbols in the threadPrivateSymbols are reset and new symbols get generated ( local copies probably ). Do you think I am missing anything ? Is this ok ? Please let me know. Thank you.

@kiranchandramohan , I was working with the following test case you have given ,

In D92732#2562305, @kiranchandramohan wrote:

LGTM. I have a Nit comment.

Thank you @kiranchandramohan

Patch updated and rebased after addressing the review comments.

Thanks for the review comments. @kiranchandramohan . The patch is updated after addressing the review comments.

Based on Reviewer's comments , this patch is now updated with DoWhile , Cycles and other checks. Threadprivate related checks are removed.

In D92732#2551418, @kiranchandramohan wrote:

Thanks @yhegde for the changes. I believe this threadprivate check needs some more design and work. I don' t think we can clear the threadprivate symbols in module subprograms because it will fail to catch the error in test1 below. I think the threadprivate information should be captured in a symbol so that it is available in places where the module is used like in test2.

Addressed the review comments and rebased .

This patch address the review comments.
The cycle related checks are in OmpCycleChecker class.
The test cases omp-do08.f90 and then omp-do12.f90 to omp-do17.f90 are the test cases for cycle checks.

In D92732#2537715, @kiranchandramohan wrote:

I think you are doing three patches worth of work in a single one. Ideally, we could have had, One patch for the cycle restrictions, One patch for threadprivate, and One patch for the rest.

This patch has the following changes.

1. The functions CheckIntentInPointer and CheckDefinableObjects are combined and renamed as 'CheckIntentInPointerAndDefinable'.
2. Removed CheckDependArraySection function.
3. cleaned up unwanted code around multimap which was required earlier to reuse some part of the code.
4. Added few test cases in omp-reduction02.f90 and omp-reduction07.f90 to address the review comments.
5. Removed some unnecessary code in CheckIsVarPartOfAnotherVar

The patch is updated with Cycle checks in IfConstruct and CaseConstruct with omp-do15.f90 ,omp-do16.f90 and omp-do17.f90 . The parser::Walk is out of the loop now and called only once. Cycles are mostly a concern when appear in do loops with collapse clause. Hence most of the test cases are with collapse.

Code that are related to ordered clause and cycle stmts are modified to address the review comments.

Addressed review comments.

Addressed the review comments related to loopIV.

Rebased with the latest changes.

Updated the patch with few more test cases.

Addressed the review comments.

Rebased again.

Rebased with some code changes.

With updated Reduction Clause.

With updated Reduction Clause (OmpObjectList).

Addressed review issues. Code with similar checks are reused. There are minor changes in depend clause related checks because same function is called from reduction clause related checks.

In D93205#2466522, @kiranchandramohan wrote:

Why are you changing the tests while removing XFAIL? eg. omp-do05.f90 was supposed to check chunk_size.

This patch is updated with corrections to positive test cases and indentations.

In D90697#2465189, @kiranchandramohan wrote:

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?

This patch is updated with corrections to positive cases and indentations.

This patch is updated with corrections to positive test cases and indentations.

In D93105#2461225, @kiranchandramohan wrote:

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

In D93105#2461225, @kiranchandramohan wrote:

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Rebased. Requesting the review comments.

Implementation includes a check to "the ordered clause must be present on the loop construct if any ordered region ever binds to a loop region arising from the loop construct" - along with the previous Do loop iteration variable check , Do while loop check , Do loop threadprivate check, Do loop cycle restrictions.

Test cases added for named do loops. Global variables removed.

As reviewers feel this semantic check is not needed, I am closing this review.

In D91879#2425553, @kiranchandramohan wrote:

Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting can be done at runtime. Also kindly suggest how I can go ahead with this patch.

I was saying that the check that you have introduced in this patch is a direction to the OpenMP runtime on how to assign iterations to threads. Having different chunk sizes on different loops in a parallel region is OK, it is just that the iteration to threads mapping will be different.

My recommendation is to drop this check unless you have a strong reason to not to.

In D91879#2424968, @kiranchandramohan wrote:

I only saw the following one which puts some conditions for using the same assignment of logical iteration numbers to threads for two loops. If these conditions are not met it is just that the implementation can use a different assignment. Are you referring to something else?

"A compliant implementation of the static schedule must ensure that the same assignment of logical iteration numbers to threads will be used in two loop regions if the following conditions are satisfied: 1) both loop regions have the same number of loop iterations, 2) both loop regions have the same value of chunk_size specified, or both loop regions have no chunk_size specified, 3) both loop regions bind to the same parallel region,

and 4) neither loop is associated with a SIMD construct. A data dependence between the same logical iterations in two such loops is guaranteed to be satisfied allowing safe use of the nowait clause."

This patch resolves the name symbol for the procedure, called in Reduction clause.
A new test case is added -
omp-reduction10.f90.

Committed. https://reviews.llvm.org/rG3fa20baf009d

Shall I assume there are no issues with this patch. ?

Thank you very much for the inputs @kiranchandramohan . If my understanding is correct, this test case requires a run time check and probably it is not possible to do a semantic check.

Thanks for review comments.
The code changes with this patch includes ResolvingNames for max, min, iand, ior, and ieor , assuming that the reduction clause can take user defined procedures.

Thank you both @kiranchandramohan and @clementval for your review comments and inputs.

Thanks for reviewing.
Function name PrivatizeAssociatedLoopIndex is changed to PrivatizeAssociatedLoopIndexAndCheckLoopLevel.

Thanks a lot for the review comments.
Requesting a quick review of this accepted patch. I had to revert the code part related to Shared and Private clause from the review https://reviews.llvm.org/D90324.

In D90697#2372471, @kiranchandramohan wrote:

Thanks for this patch.

Can you check what is happening with this test?

      subroutine sb(q,nx)
      integer :: q(100)
      integer :: t1, t8
!$OMP PARALLEL DO PRIVATE(k,t1) REDUCTION(max:t8)
      do k=1,nx
        t1=q(k)
        t8=max(t8,t1)
      enddo
      end

1. Clause names are changed to uppercase in test cases.
2. An optional StringRef paramName is introduced. Non optional paramName would break the previous test cases.