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.

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

[flang][openacc] Do not check for unlabelled CYCLE branching #73839

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

clementval
Copy link
Contributor

There is no such restriction for any OpenACC construct. This patch adds a constexpr condition on the type of Directive.

@llvmbot llvmbot added flang Flang issues not falling into any other category openacc flang:semantics labels Nov 29, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 29, 2023

@llvm/pr-subscribers-openacc

@llvm/pr-subscribers-flang-semantics

Author: Valentin Clement (バレンタイン クレメン) (clementval)

Changes

There is no such restriction for any OpenACC construct. This patch adds a constexpr condition on the type of Directive.


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

5 Files Affected:

  • (modified) flang/lib/Semantics/check-directive-structure.h (+19-14)
  • (modified) flang/test/Semantics/OpenACC/acc-kernels-loop.f90 (+5)
  • (modified) flang/test/Semantics/OpenACC/acc-loop.f90 (+7)
  • (modified) flang/test/Semantics/OpenACC/acc-parallel-loop-validity.f90 (+5)
  • (modified) flang/test/Semantics/OpenACC/acc-serial-loop.f90 (+5)
diff --git a/flang/lib/Semantics/check-directive-structure.h b/flang/lib/Semantics/check-directive-structure.h
index fec677500722e4c..b6a6b25764fa277 100644
--- a/flang/lib/Semantics/check-directive-structure.h
+++ b/flang/lib/Semantics/check-directive-structure.h
@@ -15,6 +15,7 @@
 #include "flang/Common/enum-set.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/tools.h"
+#include <type_traits>
 #include <unordered_map>
 
 namespace Fortran::semantics {
@@ -62,20 +63,24 @@ template <typename D> class NoBranchingEnforce {
     if (const auto &cycleName{cycleStmt.v}) {
       CheckConstructNameBranching("CYCLE", cycleName.value());
     } else {
-      switch ((llvm::omp::Directive)currentDirective_) {
-      // exclude directives which do not need a check for unlabelled CYCLES
-      case llvm::omp::Directive::OMPD_do:
-      case llvm::omp::Directive::OMPD_simd:
-      case llvm::omp::Directive::OMPD_parallel_do:
-      case llvm::omp::Directive::OMPD_parallel_do_simd:
-      case llvm::omp::Directive::OMPD_distribute_parallel_do:
-      case llvm::omp::Directive::OMPD_distribute_parallel_do_simd:
-      case llvm::omp::Directive::OMPD_distribute_parallel_for:
-      case llvm::omp::Directive::OMPD_distribute_simd:
-      case llvm::omp::Directive::OMPD_distribute_parallel_for_simd:
-        return;
-      default:
-        break;
+      if constexpr (std::is_same_v<D, llvm::omp::Directive>) {
+        switch ((llvm::omp::Directive)currentDirective_) {
+        // exclude directives which do not need a check for unlabelled CYCLES
+        case llvm::omp::Directive::OMPD_do:
+        case llvm::omp::Directive::OMPD_simd:
+        case llvm::omp::Directive::OMPD_parallel_do:
+        case llvm::omp::Directive::OMPD_parallel_do_simd:
+        case llvm::omp::Directive::OMPD_distribute_parallel_do:
+        case llvm::omp::Directive::OMPD_distribute_parallel_do_simd:
+        case llvm::omp::Directive::OMPD_distribute_parallel_for:
+        case llvm::omp::Directive::OMPD_distribute_simd:
+        case llvm::omp::Directive::OMPD_distribute_parallel_for_simd:
+          return;
+        default:
+          break;
+        }
+      } else if constexpr (std::is_same_v<D, llvm::acc::Directive>) {
+        return; // OpenACC construct do not need check for unlabelled CYCLES
       }
       CheckConstructNameBranching("CYCLE");
     }
diff --git a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90 b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
index 1a280f7c54f5cd3..8653978fb6249db 100644
--- a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
+++ b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
@@ -290,4 +290,9 @@ program openacc_kernels_loop_validity
     a(i) = 3.14
   end do
 
+  !$acc parallel loop
+  do i = 1, N
+    if(i == 10) cycle
+  end do
+
 end program openacc_kernels_loop_validity
diff --git a/flang/test/Semantics/OpenACC/acc-loop.f90 b/flang/test/Semantics/OpenACC/acc-loop.f90
index ee1e6e0cce09740..f5d1e501a2b329d 100644
--- a/flang/test/Semantics/OpenACC/acc-loop.f90
+++ b/flang/test/Semantics/OpenACC/acc-loop.f90
@@ -276,4 +276,11 @@ program openacc_loop_validity
     end do
   end do
 
+  !$acc parallel
+  !$acc loop
+  do i = 1, n
+    if(i == 10) cycle
+  end do
+  !$acc end parallel
+
 end program openacc_loop_validity
diff --git a/flang/test/Semantics/OpenACC/acc-parallel-loop-validity.f90 b/flang/test/Semantics/OpenACC/acc-parallel-loop-validity.f90
index b0be7d31e4426da..7f33f9e145110c4 100644
--- a/flang/test/Semantics/OpenACC/acc-parallel-loop-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-parallel-loop-validity.f90
@@ -136,4 +136,9 @@ program openacc_parallel_loop_validity
     reduction_l = d(i) .neqv. e(i)
   end do
 
+  !$acc parallel loop
+  do i = 1, N
+    if(i == 10) cycle
+  end do
+
 end program openacc_parallel_loop_validity
diff --git a/flang/test/Semantics/OpenACC/acc-serial-loop.f90 b/flang/test/Semantics/OpenACC/acc-serial-loop.f90
index bef117b71f9312f..2832274680eca38 100644
--- a/flang/test/Semantics/OpenACC/acc-serial-loop.f90
+++ b/flang/test/Semantics/OpenACC/acc-serial-loop.f90
@@ -106,4 +106,9 @@ program openacc_serial_loop_validity
   end do
   !$acc end serial
 
+  !$acc serial loop
+  do i = 1, n
+    if(i == 10) cycle
+  end do
+
 end program openacc_serial_loop_validity

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.

Thank you!

@clementval clementval merged commit a87ca1b into llvm:main Nov 29, 2023
3 checks passed
@clementval clementval deleted the acc_cycle_check branch November 29, 2023 23:37
break;
}
} else if constexpr (std::is_same_v<D, llvm::acc::Directive>) {
return; // OpenACC construct do not need check for unlabelled CYCLES
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we will still report an error for this:

subroutine test
  do i=1,100
     !$acc data
     if (i == 10) cycle
     !$acc end data
  end do
end subroutine test

Or will it be handled elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! We are not catching this anymore. Let me send another patch since this one was merge. I'll add a whitelist of construct to skip like it is done for OpenMP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clementval added a commit that referenced this pull request Nov 30, 2023
)

Unlike mentioned in #73839, some OpenACC construct still need the cycle
branching check to be performed. This patch adds a list of construct
where the check is not needed (loop and combined construct) and add
tests to check where it is still needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:semantics flang Flang issues not falling into any other category openacc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants