This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic checks of nesting of region about ordered construct
ClosedPublic

Authored by peixin on Nov 8 2021, 6:11 AM.

Details

Summary

This patch supports the following checks for ORDERED construct:

[5.1] 2.19.9 ORDERED Construct
The worksharing-loop or worksharing-loop SIMD region to which an ordered
region corresponding to an ordered construct without a depend clause
binds must have an ordered clause without the parameter specified on the 
corresponding worksharing-loop or worksharing-loop SIMD directive.
The worksharing-loop region to which an ordered region that corresponds
to an ordered construct with any depend clauses binds must have an
ordered clause with the parameter specified on the corresponding
worksharing-loop directive.
An ordered construct with the depend clause specified must be closely
nested inside a worksharing-loop (or parallel worksharing-loop)
construct.
An ordered region that corresponds to an ordered construct with the simd
clause specified must be closely nested inside a simd or
worksharing-loop SIMD region.

Diff Detail

Event Timeline

peixin created this revision.Nov 8 2021, 6:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
peixin requested review of this revision.Nov 8 2021, 6:11 AM
peixin added a comment.Nov 8 2021, 6:20 AM

The following two checks cannot be fully supported because of limited inter-procedure analysis.

An ordered region that corresponds to an ordered construct without the simd clause specified must be closely nested inside a worksharing-loop region.
During execution of an iteration of a worksharing-loop or a loop nest within a worksharing-loop, simd, or worksharing-loop SIMD region, a thread must not execute more than one ordered region that corresponds to an ordered construct without a depend clause.

Up to now, all the semantic checks of nesting of regions about ordered construct nested in worksharing-loop, SIMD, or worksharing-loop SIMD region are supported.

LGTM. I had concerns about removal of OmpStructureChecker::CheckIfDoOrderedClause but I guess condensing all checks in one function is a better decision. We could wait for others' comments before landing the patch

Hi @peixin. Thanks for the patch. I had a few comments.

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

There are too many branches inside the for loop. Can we make it of the form?

for(...) {
  isNestedInDo = ...
  isNestedInSIMD = ...
  ... <same for other bool variables> ...
  if (isNestedInDo || isNestedInSIMD || ...)
    break;
}

That would be more readable and will reduce the branching in this code.

774–775

Suggestion: Considering changing this to

isNestedInDoSIMD = llvm::omp::doSimdSet.test(dirContext_[i].directive)
780–782

Can we change this to

isOrderedClauseWithPara = GetIntValue(orderedClause.v) > 0;
784

Instead of having the bool noOrderedClause we can probably save the clause (Line #776) as a nullptr which can be used for checking later by simply checking !clause. (again, aiming at reducing the branches in code to make it more readable).

794

There is no test case for this.

818

Nit: error message is inconsistent with others (others do not use parentheses).

flang/test/Semantics/omp-do06.f90
24

As far as I can tell there is no "good" testcase for this. Like

!$omp do ordered
do i = 1, 10
  print *, i
  !$omp ordered
      print *,i
  !$omp end ordered
end do
!$omp end do

Can we maybe add such testcase, if it is not already there?

peixin updated this revision to Diff 397845.Jan 6 2022, 3:58 AM

Update and rebase.

peixin added a comment.Jan 6 2022, 4:09 AM

Thanks @shraiysh for the review. Addressed the comments.

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

These bool variables cannot declared inside the for loop. They are collected when going through all the nested direcitives and checked to emit the error messages later.

774–775

Fixed.

780–782

Fixed.

784

noOrderedClause defines what it means. I think it's better to store one bool variable instead of one class object. The branches cannot be reduced even if save the clause.

794

This check is moved from CheckIfDoOrderedClause and the test case omp-ordered-simd.f90 has been added before.

818

Please check the commit message. These error messages are from the standard. I use the original restrictions of the standard.

flang/test/Semantics/omp-do06.f90
24

Added in omp_ordered02.f90.

shraiysh accepted this revision.Jan 9 2022, 11:32 PM

Thanks for addressing the comments @peixin. One minor comment, rest LGTM. (Please wait for one more review because I am not an expert in this part of the code).

flang/lib/Semantics/check-omp-structure.cpp
771–799

I meant changing it this way. This will eliminate constructs like this -

if(condition)
  var = true;

by setting var = condition. This change passes all the tests in the current patch. Please let me know if I have missed something here.

784

Okay.

794

Oh okay, thanks for the clarification.

This revision is now accepted and ready to land.Jan 9 2022, 11:32 PM

Thanks for addressing the comments @peixin. One minor comment, rest LGTM. (Please wait for one more review because I am not an expert in this part of the code).

Sure. Thanks for the notice.

flang/lib/Semantics/check-omp-structure.cpp
771–799

Oh. This change sacrifice the time cost. For example, isOrderedClauseWithPara is true for !$omp simd ordered(2). Actually, the code inside the if condition of (llvm::omp::doSet.test(dirContext_[i].directive)) should never be run for this case. So, the left if conditions are useful to avoid extra operations. What do you think?

peixin updated this revision to Diff 398570.Jan 10 2022, 4:04 AM

Reformat the if conditions.

peixin added inline comments.Jan 10 2022, 4:06 AM
flang/lib/Semantics/check-omp-structure.cpp
771–799

@shraiysh I reformat these checks. The current code should be more readable.

Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 4:24 AM
kiranchandramohan requested changes to this revision.Apr 12 2022, 10:27 AM

I have a few comments, mostly Nits.

flang/lib/Semantics/check-directive-structure.h
271

Can you move this closer to the FindClause function above and define the above FindClause function in terms of this?

const PC *FindClause(C type) {
  return FindClause(GetContext(), type);
}
flang/lib/Semantics/check-omp-structure.cpp
764–769

Nit: for all above please use braced initialization

808

Nit: Use curlies

812

Nit: Use curlies

1102

Nit: curlies

This revision now requires changes to proceed.Apr 12 2022, 10:27 AM
peixin updated this revision to Diff 422470.Apr 13 2022, 4:28 AM

Addressed comments from @kiranchandramohan .

LGTM. Thanks @peixin and apologies for the long wait.

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

Nit: braced init.

1096

Nit: Braced init if possible.

This revision is now accepted and ready to land.Apr 13 2022, 5:00 AM
peixin updated this revision to Diff 422484.Apr 13 2022, 5:12 AM

Fix two nits and rebase to test CI before land.