Skip to content
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

Jump to bottom

[flang][openacc] Check atomic update lhs/rhs are scalar #66113

Merged
merged 1 commit into from
Sep 12, 2023
Merged

[flang][openacc] Check atomic update lhs/rhs are scalar #66113

merged 1 commit into from
Sep 12, 2023

Conversation

clementval
Copy link
Contributor

@clementval clementval commented Sep 12, 2023 •
edited

Add semantic check for !$acc atomic update. RHS and LHS of the update assignment must be scalar.

Without this check the code was going all the way to MLIR and trigger a verifier error. This gives better user error message.

@clementval clementval requested a review from a team as a code owner September 12, 2023 17:27
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:semantics openacc labels Sep 12, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 12, 2023

@llvm/pr-subscribers-flang-semantics

Changes

Add semantic check for !$acc atomic update. RHS and LHS of the update assignment must be scalar.

Full diff: https://github.com/llvm/llvm-project/pull/66113.diff

3 Files Affected:

  • (modified) flang/lib/Semantics/check-acc-structure.cpp (+17)
  • (modified) flang/lib/Semantics/check-acc-structure.h (+1)
  • (modified) flang/test/Semantics/OpenACC/acc-atomic-validity.f90 (+7)
diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 4ae034736a40b09..3c9e89940d23784 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -331,6 +331,23 @@ void AccStructureChecker::Leave(const parser::OpenACCAtomicConstruct &x) {
   dirContext_.pop_back();
 }
 
+void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
+  const parser::AssignmentStmt &assignment{
+      std::get>(x.t).statement};
+  const auto &var{std::get(assignment.t)};
+  const auto &expr{std::get(assignment.t)};
+  const auto *rhs{GetExpr(context_, expr)};
+  const auto *lhs{GetExpr(context_, var)};
+  if (lhs && rhs) {
+    if (lhs->Rank() != 0)
+      context_.Say(expr.source,
+          "LHS of atomic update statement must be scalar"_err_en_US);
+    if (rhs->Rank() != 0)
+      context_.Say(var.GetSource(),
+          "RHS of atomic update statement must be scalar"_err_en_US);
+  }
+}
+
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {
   const auto &verbatim = std::get(x.t);
   PushContextAndClauseSets(verbatim.source, llvm::acc::Directive::ACCD_cache);
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 6d05acba92cb268..8b87b8ddc502f3f 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -62,6 +62,7 @@ class AccStructureChecker
   void Leave(const parser::OpenACCAtomicConstruct &);
   void Enter(const parser::OpenACCCacheConstruct &);
   void Leave(const parser::OpenACCCacheConstruct &);
+  void Enter(const parser::AccAtomicUpdate &);
 
   // Clauses
   void Leave(const parser::AccClauseList &);
diff --git a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90 b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
index e6d82a77f6c1bc6..ba68031b0f18bc7 100644
--- a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
@@ -11,6 +11,7 @@ program openacc_atomic_validity
   integer, parameter :: N = 256
   integer, dimension(N) :: c
 
+
   !$acc parallel
   !$acc atomic update
   c(i) = c(i) + 1
@@ -37,6 +38,12 @@ program openacc_atomic_validity
   c(i) = i
   i = i + 1
   !$acc end atomic
+
+  !$acc atomic update
+  !ERROR: RHS of atomic update statement must be scalar
+  !ERROR: LHS of atomic update statement must be scalar
+  c = c + 1
+
   !$acc end parallel
 
 end program openacc_atomic_validity

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 12, 2023

@llvm/pr-subscribers-openacc

Changes

Add semantic check for !$acc atomic update. RHS and LHS of the update assignment must be scalar.

Full diff: https://github.com/llvm/llvm-project/pull/66113.diff

3 Files Affected:

  • (modified) flang/lib/Semantics/check-acc-structure.cpp (+17)
  • (modified) flang/lib/Semantics/check-acc-structure.h (+1)
  • (modified) flang/test/Semantics/OpenACC/acc-atomic-validity.f90 (+7)
diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 4ae034736a40b09..3c9e89940d23784 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -331,6 +331,23 @@ void AccStructureChecker::Leave(const parser::OpenACCAtomicConstruct &x) {
   dirContext_.pop_back();
 }
 
+void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
+  const parser::AssignmentStmt &assignment{
+      std::get>(x.t).statement};
+  const auto &var{std::get(assignment.t)};
+  const auto &expr{std::get(assignment.t)};
+  const auto *rhs{GetExpr(context_, expr)};
+  const auto *lhs{GetExpr(context_, var)};
+  if (lhs && rhs) {
+    if (lhs->Rank() != 0)
+      context_.Say(expr.source,
+          "LHS of atomic update statement must be scalar"_err_en_US);
+    if (rhs->Rank() != 0)
+      context_.Say(var.GetSource(),
+          "RHS of atomic update statement must be scalar"_err_en_US);
+  }
+}
+
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {
   const auto &verbatim = std::get(x.t);
   PushContextAndClauseSets(verbatim.source, llvm::acc::Directive::ACCD_cache);
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 6d05acba92cb268..8b87b8ddc502f3f 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -62,6 +62,7 @@ class AccStructureChecker
   void Leave(const parser::OpenACCAtomicConstruct &);
   void Enter(const parser::OpenACCCacheConstruct &);
   void Leave(const parser::OpenACCCacheConstruct &);
+  void Enter(const parser::AccAtomicUpdate &);
 
   // Clauses
   void Leave(const parser::AccClauseList &);
diff --git a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90 b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
index e6d82a77f6c1bc6..ba68031b0f18bc7 100644
--- a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
@@ -11,6 +11,7 @@ program openacc_atomic_validity
   integer, parameter :: N = 256
   integer, dimension(N) :: c
 
+
   !$acc parallel
   !$acc atomic update
   c(i) = c(i) + 1
@@ -37,6 +38,12 @@ program openacc_atomic_validity
   c(i) = i
   i = i + 1
   !$acc end atomic
+
+  !$acc atomic update
+  !ERROR: RHS of atomic update statement must be scalar
+  !ERROR: LHS of atomic update statement must be scalar
+  c = c + 1
+
   !$acc end parallel
 
 end program openacc_atomic_validity

vzakhari
vzakhari approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

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

LGTM

@clementval clementval merged commit c46d732 into llvm:main Sep 12, 2023
5 checks passed
@clementval clementval deleted the acc_atomic_update_sema1 branch September 12, 2023 18:00
@vzakhari vzakhari mentioned this pull request Sep 12, 2023
Closed
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
@clementval @ZijunZhaoCCK
[flang][openacc] Check atomic update lhs/rhs are scalar (llvm#66113)
5f8aaa8 
Add semantic check for `!$acc atomic update`. RHS and LHS of the update
assignment must be scalar.

Without this check the code was going all the way to MLIR and trigger a
verifier error. This gives better user error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vzakhari vzakhari vzakhari approved these changes

@razvanlupusoru razvanlupusoru Awaiting requested review from razvanlupusoru

Assignees
No one assigned
Labels
flang Flang issues not falling into any other category openacc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@clementval @llvmbot @vzakhari