Skip to content

[flang][OpenMP]Add support for fail clause #118683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 11, 2024
Merged

[flang][OpenMP]Add support for fail clause #118683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59752d2
[flang][OpenMP]Add support for fail clause
Leporacanthicus Dec 4, 2024
04e8fd5
Fix review comments
Leporacanthicus Dec 5, 2024
35856c1
Minor review updates
Leporacanthicus Dec 6, 2024
c25a059
Refactor avoiding checking the memory order
Leporacanthicus Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flang/include/flang/Parser/dump-parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ class ParseTreeDumper {
NODE(parser, OmpEndCriticalDirective)
NODE(parser, OmpEndLoopDirective)
NODE(parser, OmpEndSectionsDirective)
NODE(parser, OmpFailClause)
NODE(parser, OmpFromClause)
NODE(OmpFromClause, Modifier)
NODE(parser, OmpExpectation)
Expand Down
14 changes: 12 additions & 2 deletions flang/include/flang/Parser/parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ struct OpenACCRoutineConstruct;
struct OpenMPConstruct;
struct OpenMPDeclarativeConstruct;
struct OmpEndLoopDirective;
struct OmpMemoryOrderClause;
struct CUFKernelDoConstruct;

// Cooked character stream locations
Expand Down Expand Up @@ -4098,6 +4099,14 @@ struct OmpUpdateClause {
std::variant<OmpDependenceType, OmpTaskDependenceType> u;
};

// OMP 5.2 15.8.3 extened-atomic, fail-clause ->
// FAIL(memory-order)
struct OmpFailClause {
WRAPPER_CLASS_BOILERPLATE(
OmpFailClause, common::Indirection<OmpMemoryOrderClause>);
CharBlock source;
};

Copy link
Contributor

@kparzysz kparzysz Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OmpXyzClause classes are ordered alphabetically. Could you move it to the right location?

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// OpenMP Clauses
struct OmpClause {
UNION_CLASS_BOILERPLATE(OmpClause);
Expand Down Expand Up @@ -4317,11 +4326,12 @@ struct OmpMemoryOrderClause {
};

// 2.17.7 Atomic construct
// atomic-clause -> memory-order-clause | HINT(hint-expression)
// atomic-clause -> memory-order-clause | HINT(hint-expression) |
// FAIL(memory-order)
struct OmpAtomicClause {
UNION_CLASS_BOILERPLATE(OmpAtomicClause);
CharBlock source;
std::variant<OmpMemoryOrderClause, OmpClause> u;
std::variant<OmpMemoryOrderClause, OmpFailClause, OmpClause> u;
};

// atomic-clause-list -> [atomic-clause, [atomic-clause], ...]
Expand Down
4 changes: 4 additions & 0 deletions flang/lib/Parser/openmp-parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,9 @@ TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))

TYPE_PARSER(sourced(construct<OmpFailClause>(
parenthesized(indirect(Parser<OmpMemoryOrderClause>{})))))

// 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
// memory-order-clause ->
// seq_cst
Expand Down Expand Up @@ -767,6 +770,7 @@ TYPE_PARSER(construct<OmpAtomicDefaultMemOrderClause>(
// atomic-clause -> memory-order-clause | HINT(hint-expression)
TYPE_PARSER(sourced(construct<OmpAtomicClause>(
construct<OmpAtomicClause>(Parser<OmpMemoryOrderClause>{}) ||
construct<OmpAtomicClause>("FAIL" >> Parser<OmpFailClause>{}) ||
construct<OmpAtomicClause>("HINT" >>
sourced(construct<OmpClause>(
construct<OmpClause::Hint>(parenthesized(constantExpr))))))))
Expand Down
6 changes: 6 additions & 0 deletions flang/lib/Parser/unparse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2702,10 +2702,16 @@ class UnparseVisitor {
Put("\n");
EndOpenMP();
}
void Unparse(const OmpFailClause &x) {
Word("FAIL(");
Walk(x.v);
Put(")");
}
void Unparse(const OmpMemoryOrderClause &x) { Walk(x.v); }
void Unparse(const OmpAtomicClause &x) {
common::visit(common::visitors{
[&](const OmpMemoryOrderClause &y) { Walk(y); },
[&](const OmpFailClause &y) { Walk(y); },
[&](const OmpClause &z) { Walk(z); },
},
x.u);
Expand Down
19 changes: 15 additions & 4 deletions flang/lib/Semantics/check-omp-structure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2486,17 +2486,28 @@ void OmpStructureChecker::CheckAtomicMemoryOrderClause(
const parser::OmpAtomicClauseList *leftHandClauseList,
const parser::OmpAtomicClauseList *rightHandClauseList) {
int numMemoryOrderClause = 0;
int numFailClause = 0;
Copy link
Contributor

@kiranchandramohan kiranchandramohan Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: braced initialization in frontend code.

auto checkForValidMemoryOrderClause =
[&](const parser::OmpAtomicClauseList *clauseList) {
for (const auto &clause : clauseList->v) {
if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
numMemoryOrderClause++;
if (numMemoryOrderClause > 1) {
if (std::get_if<parser::OmpFailClause>(&clause.u)) {
numFailClause++;
if (numFailClause > 1) {
context_.Say(clause.source,
"More than one memory order clause not allowed on "
"More than one fail clause not allowed on "
Copy link
Contributor

@kparzysz kparzysz Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you spell FAIL in all capital letters? This is the convention in diagnostic messages.

Also, could you concatenate the strings into a single line. The line will be "too long", but we do that to make grepping for messages in the sources easier.

Finally, please replace the "OpenMP Atomic construct" with "ATOMIC construct".

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on all three parts. This was a bit of a "copy-paste" thing, so I just followed what was already there for memory order, but of course, memory order isn't a "keyword" the same way that FAIL is. The split strings is annoying me quite often, so thanks for pointing that out - again, copying what was already there (poor excuse, but it's the best I've got!)

"OpenMP Atomic construct"_err_en_US);
return;
}
} else {
if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
numMemoryOrderClause++;
if (numMemoryOrderClause > 1) {
context_.Say(clause.source,
"More than one memory order clause not allowed on "
"OpenMP Atomic construct"_err_en_US);
Copy link
Contributor

@kparzysz kparzysz Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here---please concatenate and use "ATOMIC construct".

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return;
}
}
}
}
};
Expand Down
5 changes: 5 additions & 0 deletions flang/lib/Semantics/semantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ class SemanticsVisitor : public virtual BaseChecker, public virtual C... {
context_.set_location(std::nullopt);
}

// This is necessary to avoid "walking" into the Fail clause,
// which confuses the CheckAllowed into thinking there's another
// memoryorder, when it's actually the argument to the fail clause.
bool Pre(const parser::OmpFailClause &) { return false; }
Copy link
Contributor

@kiranchandramohan kiranchandramohan Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this disallow any future checks for the OmpFailClause?

What are the alternatives here?
Marking the MemoryOrder values for OmpFail as an ENUM or something?
Alternative, would be to manually check the clauses having MemoryOrder components in check-omp-structure.cpp.

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we have the correct parser, there is no way that the FAIL clause will contain anything other than a MemoryOrder.

The problem we're trying to solve here is that the default is to Walk everything where Pre returns true. So if we have !$OMP ATOMIC COMPARE SEQ_CST FAIL(RELAXED), it will get the the FAIL, and default Pre returns true, so it gets to process the RELAXED. It treats that as if it was part of the regular list of memory order objects listed, and says "There's two memory-orders on the same ATOMIC COMPARE" (not the precise wording).

We COULD use a different type, and then have a separate checker bit, but than we will also end up with two (almost) identical parsers for memory order objects, and I'm not sure exactly what it solves to do that.

We can always call some function inside Pre that checks things. Or call check the fail clause in the check-omp-structure.cpp.

Really, none of the solutions are fantastic. This works for the moment, and I'm not sure it makes sense to add more complexity, unless we have a good reason for that.

Copy link
Contributor

@kiranchandramohan kiranchandramohan Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking whether this solution will prevent the ability to perform the following semantic check from the OpenMP 5.2 standard.

-> acq_rel and release cannot be specified as arguments to the fail clause.

If you do not want to change the parse-tree representation, an alternative solution could be to remember whether we are visiting the FailClause and only call CheckAllowedClause for the MemoryOrder clauses if we are not inside the FailClause. Something like the following (only done for Release MemoryOrder clause).

+void OmpStructureChecker::Enter(const parser::OmpFailClause &) {
+  CheckAllowedClause(llvm::omp::Clause::OMPC_fail);
+  isFailClause = true;
+}
+
+void OmpStructureChecker::Leave(const parser::OmpFailClause &) {
+  isFailClause = false;
+}
+
+void OmpStructureChecker::Enter(const parser::OmpClause::Release &) {
+  if(!isFailClause) {
+    CheckAllowedClause(llvm::omp::Clause::OMPC_release);
+  }
+}

Note: The above code will also involve removing the CHECK_SIMPLE_CLAUSE entries for the Release MemoryOrder clause, adding isFailClause as a member of OmpStructureChecker, and adding the Enter and Leave signatures in OmpStructureChecker for the OmpFailClause.

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. As far as I can tell, there's nothing automatic (in OMP.td) that provides a help for the fail clause's memory order options. We'd have to write code for that somewhere. We already do a bunch of checking for memoryorder in the semantics of ATOMIC in general, and we also need to check that the fail clause doesn't appear outside of a COMPARE (at least I think that's how it's meant to work).

The alternative suggested would be a workable solution. In fact, I'm liking that more, mostly because it doesn't alter a generic class used everywhere. I'll implement that.

In both cases, we still need to know whether we're checking a memory order inside a fail clause or not - and I guess we would add an else Say("Release memory order not allowed on FAIL clause") in the not allowed cases (not exactly how it would be written, but conceptually).


bool Walk(const parser::Program &program) {
parser::Walk(program, *this);
return !context_.AnyFatalError();
Expand Down
11 changes: 11 additions & 0 deletions flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s

! CHECK: not yet implemented: OpenMP atomic compare
program p
integer :: x
logical :: r
!$omp atomic compare fail(relaxed)
if (x .eq. 0) then
x = 2
end if
end program p
19 changes: 19 additions & 0 deletions flang/test/Parser/OpenMP/atomic-unparse.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,20 @@ program main
i = j
end if


!$omp atomic compare fail(relaxed)
if (i .eq. k) then
i = j
end if
!$omp atomic fail(relaxed) compare
if (i .eq. k) then
i = j
end if
!$omp atomic fail(relaxed) compare acquire
if (i .eq. k) then
i = j
end if

!ATOMIC
!$omp atomic
i = j
Expand Down Expand Up @@ -262,6 +276,9 @@ end program main
!CHECK: !$OMP ATOMIC COMPARE ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELAXED
!CHECK: !$OMP ATOMIC COMPARE FAIL(RELAXED)
!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE
!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE ACQUIRE

!ATOMIC
!CHECK: !$OMP ATOMIC
Expand All @@ -270,3 +287,5 @@ end program main
!CHECK: !$OMP ATOMIC ACQ_REL
!CHECK: !$OMP ATOMIC ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED


13 changes: 13 additions & 0 deletions flang/test/Semantics/OpenMP/atomic-compare.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@
if (b .eq. a) b = c
!$omp end atomic

!$omp atomic hint(1) acq_rel compare fail(release)
if (c .eq. a) a = b
!$omp end atomic

!$omp atomic compare fail(release)
if (c .eq. a) a = b
!$omp end atomic

! Check for error conditions:
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one SEQ_CST clause can appear on the COMPARE directive
Expand Down Expand Up @@ -75,5 +83,10 @@
!$omp atomic relaxed compare relaxed
if (b .eq. c) b = a

!ERROR: More than one fail clause not allowed on OpenMP Atomic construct
!$omp atomic fail(release) compare fail(release)
if (c .eq. a) a = b
!$omp end atomic

!$omp end parallel
end
1 change: 1 addition & 0 deletions llvm/include/llvm/Frontend/OpenMP/OMP.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def OMPC_Exclusive : Clause<"exclusive"> {
}
def OMPC_Fail : Clause<"fail"> {
let clangClass = "OMPFailClause";
let flangClass = "OmpFailClause";
}
def OMPC_Filter : Clause<"filter"> {
let clangClass = "OMPFilterClause";
Expand Down
Loading