[flang][OpenMP] Error out when CHARACTER type is used in atomic constructs #113045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-openmp Author: None (NimishMishra) ChangesAccording to OpenMPv5.2 1.2.6, "For Fortran, a scalar variable with intrinsic type, as defined by the base language, excluding character type.". Likewise, section 4.3.1.3 states that atomic operations are on "scalar variables of intrinsic type". This PR hence introduces a check to error out when CHARACTER type is used in atomic operations. Fixes #112918 Full diff: https://github.com/llvm/llvm-project/pull/113045.diff 6 Files Affected:
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index da192ded4aa971..4e0e59816e8197 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -128,6 +128,7 @@ static void processOmpAtomicTODO(mlir::Type elementType,
Fortran::parser::OmpAtomicClauseList>()) {
// Based on assertion for supported element types in OMPIRBuilder.cpp
// createAtomicRead
+ // TODO: Enumerate remaining unsupported types for atomics
mlir::Type unwrappedEleTy = fir::unwrapRefType(elementType);
bool supportedAtomicType = fir::isa_trivial(unwrappedEleTy);
if (!supportedAtomicType)
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 461a99f59e4ce7..6479d15821d14b 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -1888,16 +1888,21 @@ inline void OmpStructureChecker::ErrIfLHSAndRHSSymbolsMatch(
inline void OmpStructureChecker::ErrIfNonScalarAssignmentStmt(
const parser::Variable &var, const parser::Expr &expr) {
// Err out if either the variable on the LHS or the expression on the RHS of
- // the assignment statement are non-scalar (i.e. have rank > 0)
+ // the assignment statement are non-scalar (i.e. have rank > 0 or is of
+ // CHARACTER type)
const auto *e{GetExpr(context_, expr)};
const auto *v{GetExpr(context_, var)};
if (e && v) {
- if (e->Rank() != 0)
+ if (e->Rank() != 0 ||
+ (e->GetType().has_value() &&
+ e->GetType().value().category() == common::TypeCategory::Character))
context_.Say(expr.source,
"Expected scalar expression "
"on the RHS of atomic assignment "
"statement"_err_en_US);
- if (v->Rank() != 0)
+ if (v->Rank() != 0 ||
+ (v->GetType().has_value() &&
+ v->GetType()->category() == common::TypeCategory::Character))
context_.Say(var.GetSource(),
"Expected scalar variable "
"on the LHS of atomic assignment "
@@ -2008,12 +2013,16 @@ void OmpStructureChecker::CheckAtomicUpdateStmt(
expr.u);
if (const auto *e{GetExpr(context_, expr)}) {
const auto *v{GetExpr(context_, var)};
- if (e->Rank() != 0)
+ if (e->Rank() != 0 ||
+ (e->GetType().has_value() &&
+ e->GetType().value().category() == common::TypeCategory::Character))
context_.Say(expr.source,
"Expected scalar expression "
"on the RHS of atomic update assignment "
"statement"_err_en_US);
- if (v->Rank() != 0)
+ if (v->Rank() != 0 ||
+ (v->GetType().has_value() &&
+ v->GetType()->category() == common::TypeCategory::Character))
context_.Say(var.GetSource(),
"Expected scalar variable "
"on the LHS of atomic update assignment "
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-character.f90 b/flang/test/Lower/OpenMP/Todo/atomic-character.f90
deleted file mode 100644
index 88effa4a2a5156..00000000000000
--- a/flang/test/Lower/OpenMP/Todo/atomic-character.f90
+++ /dev/null
@@ -1,8 +0,0 @@
-! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
-
-! CHECK: not yet implemented: Unsupported atomic type
-subroutine character_atomic
- character :: l, r
- !$omp atomic read
- l = r
-end subroutine
diff --git a/flang/test/Semantics/OpenMP/atomic02.f90 b/flang/test/Semantics/OpenMP/atomic02.f90
index b823bc4c33b239..57a97d88b9e9b3 100644
--- a/flang/test/Semantics/OpenMP/atomic02.f90
+++ b/flang/test/Semantics/OpenMP/atomic02.f90
@@ -32,6 +32,7 @@ program OmpAtomic
a = a**4
!$omp atomic
!ERROR: Invalid or missing operator in atomic update statement
+ !ERROR: Expected scalar expression on the RHS of atomic update assignment statement
c = c//d
!$omp atomic
!ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
@@ -78,6 +79,7 @@ program OmpAtomic
a = a**4
!$omp atomic update
!ERROR: Invalid or missing operator in atomic update statement
+ !ERROR: Expected scalar expression on the RHS of atomic update assignment statement
c = c//d
!$omp atomic update
!ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
diff --git a/flang/test/Semantics/OpenMP/atomic03.f90 b/flang/test/Semantics/OpenMP/atomic03.f90
index 76367495b98612..40347170bacd2b 100644
--- a/flang/test/Semantics/OpenMP/atomic03.f90
+++ b/flang/test/Semantics/OpenMP/atomic03.f90
@@ -129,7 +129,7 @@ subroutine more_invalid_atomic_update_stmts()
!$omp atomic update
!ERROR: Expected scalar variable on the LHS of atomic update assignment statement
- !ERROR: Intrinsic procedure arguments in atomic update statement must have exactly one occurence of 'k'
+ !ERROR: Intrinsic procedure arguments in atomic update statement must have exactly one occurence of 'k'
k = max(x, y)
!$omp atomic
diff --git a/flang/test/Semantics/OpenMP/omp-atomic-assignment-stmt.f90 b/flang/test/Semantics/OpenMP/omp-atomic-assignment-stmt.f90
index 9701c1db92c1cd..505cbc48fef901 100644
--- a/flang/test/Semantics/OpenMP/omp-atomic-assignment-stmt.f90
+++ b/flang/test/Semantics/OpenMP/omp-atomic-assignment-stmt.f90
@@ -14,6 +14,7 @@ program sample
integer :: m
endtype
type(sample_type) :: z
+ character :: l, r
!$omp atomic read
v = x
@@ -148,4 +149,14 @@ program sample
y(1) = y(1) + 1
x = y(2)
!$omp end atomic
+
+ !$omp atomic read
+ !ERROR: Expected scalar variable on the LHS of atomic assignment statement
+ !ERROR: Expected scalar expression on the RHS of atomic assignment statement
+ l = r
+
+ !$omp atomic write
+ !ERROR: Expected scalar variable on the LHS of atomic assignment statement
+ !ERROR: Expected scalar expression on the RHS of atomic assignment statement
+ l = r
end program
|
NimishMishra
commented
Oct 19, 2024
flang/lib/Lower/DirectivesCommon.h
Outdated
| @@ -128,6 +128,7 @@ static void processOmpAtomicTODO(mlir::Type elementType, | |||
| Fortran::parser::OmpAtomicClauseList>()) { | |||
| // Based on assertion for supported element types in OMPIRBuilder.cpp | |||
| // createAtomicRead | |||
| // TODO: Enumerate remaining unsupported types for atomics | |||
| mlir::Type unwrappedEleTy = fir::unwrapRefType(elementType); | |||
| bool supportedAtomicType = fir::isa_trivial(unwrappedEleTy); | |||
There was a problem hiding this comment.
Should we let processOmpAtomicTODO be? Or we are in a position to remove this? I didn't remove right now; thought to ask once.
There was a problem hiding this comment.
I agree it can be replaced by an assertion
There was a problem hiding this comment.
Just so I understand it correctly, should we replace the current checks in this function with an unconditional assertion? Or should I let isa_trivial check be as it is?
There was a problem hiding this comment.
I would replace the function with something like assert(fir::isa_trivial(fir::unwrapRefType(elementType)) && "is supported type for omp atomic")
tblah
approved these changes
Oct 21, 2024
flang/lib/Lower/DirectivesCommon.h
Outdated
| @@ -128,6 +128,7 @@ static void processOmpAtomicTODO(mlir::Type elementType, | |||
| Fortran::parser::OmpAtomicClauseList>()) { | |||
| // Based on assertion for supported element types in OMPIRBuilder.cpp | |||
| // createAtomicRead | |||
| // TODO: Enumerate remaining unsupported types for atomics | |||
| mlir::Type unwrappedEleTy = fir::unwrapRefType(elementType); | |||
| bool supportedAtomicType = fir::isa_trivial(unwrappedEleTy); | |||
There was a problem hiding this comment.
I agree it can be replaced by an assertion
NimishMishra
force-pushed
the
atomic-character
branch
from
26c0f43 to
e593c76
Compare
NimishMishra
force-pushed
the
atomic-character
branch
from
e593c76 to
9bfd15a
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/5215 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/9310 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/5286 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/5338 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/172/builds/5083 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/29/builds/5467 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/20/builds/4679 Here is the relevant piece of the build log for the reference |
|
This commit (8ad8db9) reverts a MLIR Tosa commit which caused buildbot failures. I see aarch64 flang buildbots are clean now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to OpenMPv5.2 1.2.6, "For Fortran, a scalar variable with intrinsic type, as defined by the base language, excluding character type.". Likewise, section 4.3.1.3 states that atomic operations are on "scalar variables of intrinsic type". This PR hence introduces a check to error out when CHARACTER type is used in atomic operations.
Fixes #112918