This is an archive of the discontinued LLVM Phabricator instance.

[flang][openmp]At most one threads, simd and depend clause can appear on OpenMP ORDERED construct.
Needs RevisionPublic

Authored by sameeranjoshi on Jan 5 2021, 7:29 AM.

Details

Summary

Section 2.17.9 from omp 5.0 has a few restrictions related to ORDERED construct.

  • At most one threads clause can appear on an ordered construct.
  • At most one simd clause can appear on an ordered construct.
  • At most one depend(source) clause can appear on an ordered construct.
  • Either depend(sink:vec) clauses or depend(source) clauses may appear on an ordered construct, but not both.

This patch implements them by changing the TableGen file(OMP.td).
The dependencies Simd and Threads clauses were missing semantic checks and were added.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Jan 5 2021, 7:29 AM
sameeranjoshi requested review of this revision.Jan 5 2021, 7:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
sameeranjoshi added a project: Restricted Project.Jan 5 2021, 7:30 AM

Two quick questions.

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

Will this catch two or more depend(sink: v) clauses accidentally?

At most one depend(source) clause can appear on an ordered construct.

Can threads, simd occur with depend? Is that handled by the parser?

clementval added inline comments.Jan 5 2021, 12:44 PM
flang/test/Semantics/omp-clause-validity01.f90
489

According to the standard multiple depend(sink: vec) are allowed so you will need to make a specific check for the depend clause since the general allowedOnce check does not support this.

@kiranchandramohan With the current patch threads, simd and depend are allowed together. The parser is not limiting clauses.

llvm/include/llvm/Frontend/OpenMP/OMP.td
463–464

OMPC_Depend should probably be in allowedClauses and specific check done in check-omp-structure for the difference between depend(source) and depend(sink:vec)

Summary of changes:

  • Fix core dump in ordered construct due to missing tree node visit

for OpenMPSimpleStandaloneConstruct as ordered with a depend falls under it.

  • Fix internal error in name resolution of depend clause.

Internal error for Critical construct seems to get fixed with this code change.
Not really sure how does that fix, but this will be revisited in D93051.

This diff doesn't yet work on the review comments so as to avoid confusions.

kiranchandramohan requested changes to this revision.Jan 11 2021, 2:52 PM

Requesting changes as per review comments/questions before. Thanks @clementval.

This revision now requires changes to proceed.Jan 11 2021, 2:52 PM

Summary of changes:

  • Handle depend(source) and depend(sink:vec) separately.
  • Handle one more restriction from standard[5.0][2.17.9] which was needed with this patch. Either depend(sink:vec) clauses or depend(source) clauses may appear on an ordered construct, but not both.
sameeranjoshi marked 2 inline comments as done.Jan 12 2021, 7:03 AM
clementval added inline comments.Jan 12 2021, 7:18 AM
flang/lib/Semantics/check-omp-structure.cpp
892

Why PC? You have Fortran::parser::OmpDependClause pointers directly.

sameeranjoshi edited the summary of this revision. (Show Details)Jan 12 2021, 10:13 AM
sameeranjoshi edited the summary of this revision. (Show Details)

Tweak comments as suggested.

sameeranjoshi marked an inline comment as done.Jan 12 2021, 12:01 PM
sameeranjoshi added inline comments.
flang/test/Semantics/omp-clause-validity01.f90
489

Can threads, simd occur with depend? Is that handled by the parser?

They are not supposed to occur together.
No it's not caught in parser.
This will be handled separately. I am seeing some issues need to debug it.

Ideally in the first look the checks would fit better for ordered construct
inside a single function CheckDependOnOrderedDir so we have them at a
single place , unfortunately it's harder to implement it in above function
considering that the information to differentiate between a
OmpBlockDirective and OmpSimpleStandaloneDirective is unavailable.

Hence the approach of checks inside the respective nodes was choosen.
Using FindClause doesn't work for some reason hence reverting to the old
school way of finding manually inside a list of OmpClause.

kiranchandramohan requested changes to this revision.Jan 19 2021, 3:00 PM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
223

Why is this handled on Leave and not Enter like others?

305–315

Same question here.

905–942
struct OmpDependenceType {
  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
};

struct OmpDependClause {
....
  struct InOut {
    TUPLE_CLASS_BOILERPLATE(InOut);
    std::tuple<OmpDependenceType, std::list<Designator>> t;
  };
  std::variant<Source, Sink, InOut> u;
};

Can you use OmpDependenceType in OmpDependClause to simplify this logic? I think you can just count the source, sink and print out the error messages.

This revision now requires changes to proceed.Jan 19 2021, 3:00 PM
clementval added inline comments.Jan 20 2021, 7:30 AM
flang/lib/Semantics/check-omp-structure.cpp
223

Good point.

sameeranjoshi marked an inline comment as done.Jan 20 2021, 7:33 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
223

With the current approach (traversing an OmpClauseList) it could have worked inside Enter.
But here's are a few reasons not to use

  • I prefer doing checks inside Leave, so that the checks remain at a single location and Enter does the context pushing and other stuff.
  • I had initially plan to use FindClause as that saves us from writing more code and code looks more readable, unfortunately that did not work in the first run as ResetPartialContext clears the members in DirectiveContext which has a reason because OmpBeginBlockDirective & OmpEndBlockDirective have different set of clauses based on the context.

To resolve this will add Leave for OmpBeginBlockDirective.

void Enter(const parser::OpenMPBlockConstruct &);
void Leave(const parser::OpenMPBlockConstruct &);
void Leave(const parser::OmpBeginBlockDirective &);
void Enter(const parser::OmpEndBlockDirective &);
  • FindClause doesn't work inside Enter as the list is empty and it's populated only when we visit each clause. grep FindClause and see it's not found inside any Enter function.
305–315

The case of OpenMPSimpleStandaloneConstruct is different as there is no ResetPartialContext nor any end constructs nor directives for it.
So doing the check here has below reason:

  • FindClause will only work inside Leave.
  • A style approach.

With the next diff I have plan to use FindClause.

sameeranjoshi added inline comments.Jan 20 2021, 10:44 AM
flang/lib/Semantics/check-omp-structure.cpp
905–942

Could anyone please elaborate more on above?
I couldn't understand .
parse-tree.h mentions

// 2.13.9 depend-type -> IN | OUT | INOUT | SOURCE | SINK
struct OmpDependenceType {
  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
};

Where as parsers omit's the source and sink in OmpDependenceType in openmp-parsers.cpp

TYPE_PARSER(
    construct<OmpDependenceType>("IN"_id >> pure(OmpDependenceType::Type::In) ||
        "INOUT" >> pure(OmpDependenceType::Type::Inout) ||
        "OUT" >> pure(OmpDependenceType::Type::Out)))

I was confused if the parse tree has some issues.
Hence, if you somecan elaborate a bit on what exactly is expected that would be really helpful.
Or if there's a more better way of simplifying this code.

clementval added inline comments.Jan 20 2021, 10:44 AM
flang/lib/Semantics/check-omp-structure.cpp
223

What is blocking you now to use FindClause? The ResetPartialContext should not prevent you to use FindClause since you are checking different sets of clauses.

sameeranjoshi marked an inline comment as done.Jan 20 2021, 10:52 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
223

I will send a patch for the same.
I am trying to understand one more comment
https://reviews.llvm.org/D94087#inline-889166

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

Isn't FindClause used when you are inside a clause and hence do not have access to the construct? If you do it in Enter or Leave of the construct you have access to all the clauses. You might be able to use parser::Unwrap and check whether a clause is present.

905–942

I was assuming that the dependence_type is set always. If it is not then can it be set in the parser and used? Is there any reason why it is not set?

clementval added inline comments.Jan 20 2021, 12:06 PM
flang/lib/Semantics/check-omp-structure.cpp
223

FindClause might be faster since it's a simple query on a map already populated when the clause is check if it is allowed.

Summary of changes:

  • Refactor and simplify the algorithm without changes in parser.
  • Added 2 routines for code cleanup, and help extended for the later checks. ChecksOnOrderedAsStandalone ChecksOnOrderedAsBlock
  • Use FindClause to replace manual traversal of OmpClauseList. FindClause didn't work inside Leave(const parser::OpenMPBlockConstruct &) as the DirectiveContext gets reset before we reach the node. Hence checks are now done in Leave(const parser::OmpBeginBlockDirective &)
sameeranjoshi added inline comments.Jan 21 2021, 6:22 AM
flang/lib/Semantics/check-omp-structure.cpp
905–942

I have updated to simplify the logic. I am trying to not change parser in first run as there might be some reason why that might have been designed by original authors.
I have no strong opinions to change the parser if this logic still seems harder.
Thanks for parser::Unwrap inputs that helped to simplify things @kiranchandramohan.

clementval requested changes to this revision.Jan 21 2021, 7:34 AM
clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
903

Clauses should be capitalized in message.

This revision now requires changes to proceed.Jan 21 2021, 7:34 AM

Capitalized clauses in message.

clementval added inline comments.Jan 22 2021, 7:45 AM
flang/lib/Semantics/check-omp-structure.cpp
891

What is the final reason to not use FindClause? It would be more efficient than iterate over the clause list once more.

kiranchandramohan requested changes to this revision.Jun 10 2021, 12:36 AM

Will need a rebase now.

This revision now requires changes to proceed.Jun 10 2021, 12:36 AM
peixin added a subscriber: peixin.Aug 22 2021, 7:36 PM

BTW, the error of no symbol found in depend(sink: vec) should be fixed by https://reviews.llvm.org/D108530.

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

We may should also give error infomation when threads, simd occur with depend. Otherwise, the behavior would be unspecified. For current clang codegen, it generates the IR only for depend when threads, simd occur with depend. I am implementing the code for flang codegen. I should also use the similar method as clang codegen. The method should be like this:

if (isDependPresent) {
  // lowering depend
} else if (isSimdPresent) {
  // lowering simd
} else {
  // It does not matter if isThreadsPresent is true or false since the construct behaves as if threads is specified when no clause is specified.
}

I assume that this can be checked in Leave(const parser::OmpClauseList &) by using FindClause. The condition should be like this:

if ((isThreadsPresent || isSimdPresent) && isDependPresent) {
  // error report
}

Maybe there is one better place to check this.

Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 10:49 PM
clementval resigned from this revision.Apr 6 2023, 11:19 AM