Skip to content

[flang][OpenMP]Add parsing and semantics support for ATOMIC COMPARE #117032

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.

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 6 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ class ParseTreeDumper {
NODE(parser, OmpAtomicCapture)
NODE(OmpAtomicCapture, Stmt1)
NODE(OmpAtomicCapture, Stmt2)
NODE(parser, OmpAtomicCompare)
NODE(parser, OmpAtomicRead)
NODE(parser, OmpAtomicUpdate)
NODE(parser, OmpAtomicWrite)
Expand Down
13 changes: 11 additions & 2 deletions flang/include/flang/Parser/parse-tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -4126,6 +4126,15 @@ struct OmpAtomicCapture {
t;
};

// ATOMCI COMPARE (OpenMP 5.1, Spec: 15.8.4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ATOMCI COMPARE (OpenMP 5.1, Spec: 15.8.4)
// ATOMIC COMPARE (OpenMP 5.1, Spec: 15.8.4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

struct OmpAtomicCompare {
TUPLE_CLASS_BOILERPLATE(OmpAtomicCompare);
CharBlock source;
std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
Statement<AssignmentStmt>, std::optional<OmpEndAtomic>>
t;
};

// ATOMIC
struct OmpAtomic {
TUPLE_CLASS_BOILERPLATE(OmpAtomic);
Expand All @@ -4138,11 +4147,11 @@ struct OmpAtomic {
// 2.17.7 atomic ->
// ATOMIC [atomic-clause-list] atomic-construct [atomic-clause-list] |
// ATOMIC [atomic-clause-list]
// atomic-construct -> READ | WRITE | UPDATE | CAPTURE
// atomic-construct -> READ | WRITE | UPDATE | CAPTURE | COMPARE
struct OpenMPAtomicConstruct {
UNION_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
std::variant<OmpAtomicRead, OmpAtomicWrite, OmpAtomicCapture, OmpAtomicUpdate,
OmpAtomic>
OmpAtomicCompare, OmpAtomic>
u;
};

Expand Down
4 changes: 4 additions & 0 deletions flang/lib/Lower/OpenMP/OpenMP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2896,6 +2896,10 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
parser::OmpAtomicClauseList>(
converter, atomicCapture, loc);
},
[&](const parser::OmpAtomicCompare &atomicCompare) {
mlir::Location loc = converter.genLocation(atomicCompare.source);
TODO(loc, "OpenMP atomic compare");
},
},
atomicConstruct.u);
}
Expand Down
7 changes: 7 additions & 0 deletions flang/lib/Parser/openmp-parsers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,12 @@ TYPE_PARSER("ATOMIC" >>
Parser<OmpAtomicClauseList>{} / endOmpLine, statement(assignmentStmt),
statement(assignmentStmt), Parser<OmpEndAtomic>{} / endOmpLine)))

TYPE_PARSER("ATOMIC" >>
sourced(construct<OmpAtomicCompare>(
Parser<OmpAtomicClauseList>{} / maybe(","_tok), verbatim("COMPARE"_tok),
Parser<OmpAtomicClauseList>{} / endOmpLine, statement(assignmentStmt),
maybe(Parser<OmpEndAtomic>{} / endOmpLine))))

// OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST] UPDATE [MEMORY-ORDER-CLAUSE-LIST]
TYPE_PARSER("ATOMIC" >>
sourced(construct<OmpAtomicUpdate>(
Expand All @@ -934,6 +940,7 @@ TYPE_PARSER("ATOMIC" >>
// Atomic Construct
TYPE_PARSER(construct<OpenMPAtomicConstruct>(Parser<OmpAtomicRead>{}) ||
construct<OpenMPAtomicConstruct>(Parser<OmpAtomicCapture>{}) ||
construct<OpenMPAtomicConstruct>(Parser<OmpAtomicCompare>{}) ||
construct<OpenMPAtomicConstruct>(Parser<OmpAtomicWrite>{}) ||
construct<OpenMPAtomicConstruct>(Parser<OmpAtomicUpdate>{}) ||
construct<OpenMPAtomicConstruct>(Parser<OmpAtomic>{}))
Expand Down
10 changes: 10 additions & 0 deletions flang/lib/Parser/unparse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2520,6 +2520,16 @@ class UnparseVisitor {
Word("!$OMP END ATOMIC\n");
EndOpenMP();
}
void Unparse(const OmpAtomicCompare &x) {
BeginOpenMP();
Word("!$OMP ATOMIC");
Walk(std::get<0>(x.t));
Word(" COMPARE");
Walk(std::get<2>(x.t));
Put("\n");
EndOpenMP();
Walk(std::get<Statement<AssignmentStmt>>(x.t));
}
void Unparse(const OmpAtomicRead &x) {
BeginOpenMP();
Word("!$OMP ATOMIC");
Expand Down
28 changes: 28 additions & 0 deletions flang/lib/Semantics/check-omp-structure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2408,6 +2408,24 @@ void OmpStructureChecker::CheckAtomicUpdateStmt(
ErrIfAllocatableVariable(var);
}

void OmpStructureChecker::CheckAtomicCompareConstruct(
const parser::OmpAtomicCompare &atomicCompareConstruct) {

CheckAtomicWriteStmt(std::get<parser::Statement<parser::AssignmentStmt>>(
atomicCompareConstruct.t)
.statement);

unsigned version{context_.langOptions().OpenMPVersion};
if (version < 51) {
context_.Say(atomicCompareConstruct.source,
"%s construct not allowed in %s, %s"_err_en_US,
atomicCompareConstruct.source, ThisVersion(version), TryVersion(51));
}

// TODO: More work needed here. Some of the Update restrictions need to
// be added, but Update isn't the same either.
}

// TODO: Allow cond-update-stmt once compare clause is supported.
void OmpStructureChecker::CheckAtomicCaptureConstruct(
const parser::OmpAtomicCapture &atomicCaptureConstruct) {
Expand Down Expand Up @@ -2553,6 +2571,16 @@ void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x) {
&std::get<0>(atomicCapture.t), &std::get<2>(atomicCapture.t));
CheckAtomicCaptureConstruct(atomicCapture);
},
[&](const parser::OmpAtomicCompare &atomicCompare) {
const auto &dir{std::get<parser::Verbatim>(atomicCompare.t)};
PushContextAndClauseSets(
dir.source, llvm::omp::Directive::OMPD_atomic);
CheckAtomicMemoryOrderClause(
&std::get<0>(atomicCompare.t), &std::get<2>(atomicCompare.t));
CheckHintClause<const parser::OmpAtomicClauseList>(
&std::get<0>(atomicCompare.t), &std::get<2>(atomicCompare.t));
CheckAtomicCompareConstruct(atomicCompare);
},
},
x.u);
}
Expand Down
1 change: 1 addition & 0 deletions flang/lib/Semantics/check-omp-structure.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class OmpStructureChecker
void CheckAtomicCaptureStmt(const parser::AssignmentStmt &);
void CheckAtomicWriteStmt(const parser::AssignmentStmt &);
void CheckAtomicCaptureConstruct(const parser::OmpAtomicCapture &);
void CheckAtomicCompareConstruct(const parser::OmpAtomicCompare &);
void CheckAtomicConstructStructure(const parser::OpenMPAtomicConstruct &);
void CheckDistLinear(const parser::OpenMPLoopConstruct &x);
void CheckSIMDNest(const parser::OpenMPConstruct &x);
Expand Down
16 changes: 16 additions & 0 deletions flang/test/Parser/OpenMP/atomic-compare.f90
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
! RUN: not %flang_fc1 -fopenmp-version=51 -fopenmp %s 2>&1 | FileCheck %s
! OpenMP version for documentation purposes only - it isn't used until Sema.
! This is testing for Parser errors that bail out before Sema.
program main
implicit none
integer :: i, j = 10
logical :: r

!CHECK: error: expected OpenMP construct
!$omp atomic compare write
r = i .eq. j + 1

!CHECK: error: expected end of line
!$omp atomic compare num_threads(4)
r = i .eq. j
end program main
39 changes: 39 additions & 0 deletions flang/test/Parser/OpenMP/atomic-unparse.f90
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
program main
implicit none
integer :: i, j = 10
logical :: k
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should have been "r", but given the below comments, we need another variable, I think.

!READ
!$omp atomic read
i = j
Expand Down Expand Up @@ -121,6 +122,30 @@ program main
i = j
!$omp end atomic

!COMPARE
!$omp atomic compare
r = i .eq. j
!$omp atomic seq_cst compare
r = i .eq. j
!$omp atomic compare seq_cst
r = i .eq. j
!$omp atomic release compare
r = i .eq. j
!$omp atomic compare release
r = i .eq. j
!$omp atomic acq_rel compare
r = i .eq. j
!$omp atomic compare acq_rel
r = i .eq. j
!$omp atomic acquire compare
r = i .eq. j
!$omp atomic compare acquire
r = i .eq. j
!$omp atomic relaxed compare
r = i .eq. j
!$omp atomic compare relaxed
r = i .eq. j
Copy link
Contributor

Choose a reason for hiding this comment

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

The 5.1 standard specifies the format as follows. You will have to update the tests.

if (x == e) then
  x = d
end if

if (x == e) x = d

In 5.2 standard these are described as conditional-update-atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


!ATOMIC
!$omp atomic
i = j
Expand Down Expand Up @@ -205,6 +230,20 @@ end program main
!CHECK: !$OMP ATOMIC CAPTURE RELAXED
!CHECK: !$OMP END ATOMIC

!COMPARE

!CHECK: !$OMP ATOMIC COMPARE
!CHECK: !$OMP ATOMIC SEQ_CST COMPARE
!CHECK: !$OMP ATOMIC COMPARE SEQ_CST
!CHECK: !$OMP ATOMIC RELEASE COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELEASE
!CHECK: !$OMP ATOMIC ACQ_REL COMPARE
!CHECK: !$OMP ATOMIC COMPARE ACQ_REL
!CHECK: !$OMP ATOMIC ACQUIRE COMPARE
!CHECK: !$OMP ATOMIC COMPARE ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELAXED

!ATOMIC
!CHECK: !$OMP ATOMIC
!CHECK: !$OMP ATOMIC SEQ_CST
Expand Down
76 changes: 76 additions & 0 deletions flang/test/Semantics/OpenMP/atomic-compare.f90
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=51
use omp_lib
implicit none
! Check atomic compare. This combines elements from multiple other "atomic*.f90", as
! to avoid having several files with just a few lines in them. atomic compare needs
! higher openmp version than the others, so need a separate file.


real a, b
logical r
a = 1.0
b = 2.0
!$omp parallel num_threads(4)
! First a few things that should compile without error.
!$omp atomic seq_cst, compare
r = b .ne. a

!$omp atomic seq_cst compare
r = a .ge. b
!$omp end atomic

!$omp atomic compare acquire hint(OMP_LOCK_HINT_CONTENDED)
r = a .lt. b

!$omp atomic release hint(OMP_LOCK_HINT_UNCONTENDED) compare
r = a .gt. b

!$omp atomic compare seq_cst
r = b .ne. a

!$omp atomic hint(1) acq_rel compare
r = b .eq. a
!$omp end atomic

! Check for error conidtions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: conidtions -> conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, just pushed a fix for the openmp_flags, missed this. Will update in a bit, but want to see if it passes tests first! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, test didn't work, so needed a change anyway! :) Fingers crossed I figured out what is needed now... 🤞

!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
!$omp atomic seq_cst seq_cst compare
r = a .le. b
!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
!$omp atomic compare seq_cst seq_cst
r = b .gt. a
!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
!$omp atomic seq_cst compare seq_cst
r = b .ge. b

!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one ACQUIRE clause can appear on the COMPARE directive
!$omp atomic acquire acquire compare
r = a .le. b
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one ACQUIRE clause can appear on the COMPARE directive
!$omp atomic compare acquire acquire
r = b .gt. a
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one ACQUIRE clause can appear on the COMPARE directive
!$omp atomic acquire compare acquire
r = b .ge. b

!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one RELAXED clause can appear on the COMPARE directive
!$omp atomic relaxed relaxed compare
r = a .le. b
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one RELAXED clause can appear on the COMPARE directive
!$omp atomic compare relaxed relaxed
r = b .gt. a
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one RELAXED clause can appear on the COMPARE directive
!$omp atomic relaxed compare relaxed
r = b .ge. b

!$omp end parallel
end
2 changes: 2 additions & 0 deletions flang/test/Semantics/OpenMP/atomic.f90
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
a = a + 1
!ERROR: expected 'UPDATE'
!ERROR: expected 'WRITE'
!ERROR: expected 'COMPARE'
!ERROR: expected 'CAPTURE'
!ERROR: expected 'READ'
!$omp atomic num_threads(4)
Expand All @@ -49,6 +50,7 @@

!ERROR: expected 'UPDATE'
!ERROR: expected 'WRITE'
!ERROR: expected 'COMPARE'
!ERROR: expected 'CAPTURE'
!ERROR: expected 'READ'
!$omp atomic num_threads write
Expand Down
Loading