Skip to content

[flang][openacc] fix bugs with default(none) checking #149220

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
Merged
merged 1 commit into from
Jul 18, 2025
Merged

[flang][openacc] fix bugs with default(none) checking #149220

merged 1 commit into from
Jul 18, 2025

Conversation

@akuhlens
Copy link
Contributor

@akuhlens akuhlens commented Jul 16, 2025

A report of the following code not generating an error led to fixing two bugs in directive checking.

  • We should treat CombinedConstructs as OpenACC Constructs
  • We should treat DoConstruct index variables as private.
subroutine sub(nn)
  integer :: nn, ii
  !$acc serial loop default(none)
  do ii = 1, nn
  end do
  !$acc end serial loop
end subroutine

Here nn should be flagged as needing a data clause while ii should still get one implicitly.

@akuhlens
Copy link
Contributor Author

akuhlens commented Jul 16, 2025

@clementval Not sure why there was a change in handling of do concurrent lowering with this. Any ideas how to fix that?

@akuhlens akuhlens marked this pull request as ready for review July 16, 2025 23:10
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp openacc flang:semantics labels Jul 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2025 •
edited
Loading

@llvm/pr-subscribers-flang-semantics
@llvm/pr-subscribers-openacc

@llvm/pr-subscribers-flang-openmp

Author: Andre Kuhlenschmidt (akuhlens)

Changes

A report of the following code not generating an error led to fixing two bugs in directive checking.

  • We should treat CombinedConstructs as OpenACC Constructs
  • We should treat DoConstruct index variables as private.
subroutine sub(nn)
  integer :: nn, ii
  !$acc serial loop default(none)
  do ii = 1, nn
  end do
  !$acc end serial loop
end subroutine

Here nn should be flagged as needing a data clause while ii should still get one implicitly.

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

3 Files Affected:

  • (modified) flang/lib/Semantics/resolve-directives.cpp (+15)
  • (modified) flang/test/Lower/OpenACC/acc-loop.f90 (+10-7)
  • (modified) flang/test/Semantics/OpenACC/acc-kernels-loop.f90 (+7)
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 151f4ccae634e..521c7432d9fbb 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -138,6 +138,9 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
   void Post(const parser::OpenACCBlockConstruct &) { PopContext(); }
   bool Pre(const parser::OpenACCCombinedConstruct &);
   void Post(const parser::OpenACCCombinedConstruct &) { PopContext(); }
+  void Post(const parser::AccBeginCombinedDirective &) {
+    GetContext().withinConstruct = true;
+  }
 
   bool Pre(const parser::OpenACCDeclarativeConstruct &);
   void Post(const parser::OpenACCDeclarativeConstruct &) { PopContext(); }
@@ -160,6 +163,18 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
     GetContext().withinConstruct = true;
   }
 
+  // TODO: We should probably also privatize ConcurrentBounds.
+  template <typename A>
+  bool Pre(const parser::LoopBounds<parser::ScalarName, A> &x) {
+    if (!dirContext_.empty() && GetContext().withinConstruct) {
+      if (auto *symbol{ResolveAcc(
+              x.name.thing, Symbol::Flag::AccPrivate, currScope())}) {
+        AddToContextObjectWithDSA(*symbol, Symbol::Flag::AccPrivate);
+      }
+    }
+    return true;
+  }
+
   bool Pre(const parser::OpenACCStandaloneConstruct &);
   void Post(const parser::OpenACCStandaloneConstruct &) { PopContext(); }
   void Post(const parser::AccStandaloneDirective &) {
diff --git a/flang/test/Lower/OpenACC/acc-loop.f90 b/flang/test/Lower/OpenACC/acc-loop.f90
index c6df28ec5e000..f9f5e8c2165d5 100644
--- a/flang/test/Lower/OpenACC/acc-loop.f90
+++ b/flang/test/Lower/OpenACC/acc-loop.f90
@@ -372,12 +372,15 @@ subroutine sub1(i, j, k)
 end subroutine
 
 ! CHECK: func.func @_QPsub1
-! CHECK: acc.parallel
-! CHECK: %[[DC_K:.*]] = fir.alloca i32 {bindc_name = "k"}
-! CHECK: %[[DC_J:.*]] = fir.alloca i32 {bindc_name = "j"}
-! CHECK: %[[DC_I:.*]] = fir.alloca i32 {bindc_name = "i"}
-! CHECK: %[[P_I:.*]] = acc.private varPtr(%[[DC_I]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "i"}
-! CHECK: %[[P_J:.*]] = acc.private varPtr(%[[DC_J]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "j"}
-! CHECK: %[[P_K:.*]] = acc.private varPtr(%[[DC_K]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "k"}
+! CHECK-SAME: %[[ARG_I:.*]]: !fir.ref<i32> {fir.bindc_name = "i"}
+! CHECK-SAME: %[[ARG_J:.*]]: !fir.ref<i32> {fir.bindc_name = "j"}
+! CHECK-SAME: %[[ARG_K:.*]]: !fir.ref<i32> {fir.bindc_name = "k"}
+! CHECK: %[[DC_I:.*]]:2 = hlfir.declare %[[ARG_I]] dummy_scope %0
+! CHECK: %[[DC_J:.*]]:2 = hlfir.declare %[[ARG_J]] dummy_scope %0 
+! CHECK: %[[DC_K:.*]]:2 = hlfir.declare %[[ARG_K]] dummy_scope %0 
+! CHECK: acc.parallel combined(loop)
+! CHECK: %[[P_I:.*]] = acc.private varPtr(%[[DC_I]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "i"}
+! CHECK: %[[P_J:.*]] = acc.private varPtr(%[[DC_J]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "j"}
+! CHECK: %[[P_K:.*]] = acc.private varPtr(%[[DC_K]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "k"}
 ! CHECK: acc.loop combined(parallel) private(@privatization_ref_i32 -> %[[P_I]] : !fir.ref<i32>, @privatization_ref_i32 -> %[[P_J]] : !fir.ref<i32>, @privatization_ref_i32 -> %[[P_K]] : !fir.ref<i32>) control(%{{.*}} : i32, %{{.*}} : i32, %{{.*}} : i32) = (%c1{{.*}}, %c1{{.*}}, %c1{{.*}} : i32, i32, i32) to (%c10{{.*}}, %c100{{.*}}, %c200{{.*}} : i32, i32, i32)  step (%c1{{.*}}, %c1{{.*}}, %c1{{.*}} : i32, i32, i32)
 ! CHECK: } attributes {inclusiveUpperbound = array<i1: true, true, true>, independent = [#acc.device_type<none>]}
diff --git a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90 b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
index 29985a02eb6ef..cfe27e4f8fca1 100644
--- a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
+++ b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
@@ -243,8 +243,15 @@ program openacc_kernels_loop_validity
     a(i) = 3.14
   end do
 
+  !$acc kernels loop default(none) private(N, a)
+  do i = 1, N
+    a(i) = 3.14
+  end do
+
   !$acc kernels loop default(none)
+  !ERROR: The DEFAULT(NONE) clause requires that 'n' must be listed in a data-mapping clause
   do i = 1, N
+    !ERROR: The DEFAULT(NONE) clause requires that 'a' must be listed in a data-mapping clause
     a(i) = 3.14
   end do

@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2025

@llvm/pr-subscribers-flang-fir-hlfir

Author: Andre Kuhlenschmidt (akuhlens)

Changes

A report of the following code not generating an error led to fixing two bugs in directive checking.

  • We should treat CombinedConstructs as OpenACC Constructs
  • We should treat DoConstruct index variables as private.
subroutine sub(nn)
  integer :: nn, ii
  !$acc serial loop default(none)
  do ii = 1, nn
  end do
  !$acc end serial loop
end subroutine

Here nn should be flagged as needing a data clause while ii should still get one implicitly.

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

3 Files Affected:

  • (modified) flang/lib/Semantics/resolve-directives.cpp (+15)
  • (modified) flang/test/Lower/OpenACC/acc-loop.f90 (+10-7)
  • (modified) flang/test/Semantics/OpenACC/acc-kernels-loop.f90 (+7)
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 151f4ccae634e..521c7432d9fbb 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -138,6 +138,9 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
   void Post(const parser::OpenACCBlockConstruct &) { PopContext(); }
   bool Pre(const parser::OpenACCCombinedConstruct &);
   void Post(const parser::OpenACCCombinedConstruct &) { PopContext(); }
+  void Post(const parser::AccBeginCombinedDirective &) {
+    GetContext().withinConstruct = true;
+  }
 
   bool Pre(const parser::OpenACCDeclarativeConstruct &);
   void Post(const parser::OpenACCDeclarativeConstruct &) { PopContext(); }
@@ -160,6 +163,18 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
     GetContext().withinConstruct = true;
   }
 
+  // TODO: We should probably also privatize ConcurrentBounds.
+  template <typename A>
+  bool Pre(const parser::LoopBounds<parser::ScalarName, A> &x) {
+    if (!dirContext_.empty() && GetContext().withinConstruct) {
+      if (auto *symbol{ResolveAcc(
+              x.name.thing, Symbol::Flag::AccPrivate, currScope())}) {
+        AddToContextObjectWithDSA(*symbol, Symbol::Flag::AccPrivate);
+      }
+    }
+    return true;
+  }
+
   bool Pre(const parser::OpenACCStandaloneConstruct &);
   void Post(const parser::OpenACCStandaloneConstruct &) { PopContext(); }
   void Post(const parser::AccStandaloneDirective &) {
diff --git a/flang/test/Lower/OpenACC/acc-loop.f90 b/flang/test/Lower/OpenACC/acc-loop.f90
index c6df28ec5e000..f9f5e8c2165d5 100644
--- a/flang/test/Lower/OpenACC/acc-loop.f90
+++ b/flang/test/Lower/OpenACC/acc-loop.f90
@@ -372,12 +372,15 @@ subroutine sub1(i, j, k)
 end subroutine
 
 ! CHECK: func.func @_QPsub1
-! CHECK: acc.parallel
-! CHECK: %[[DC_K:.*]] = fir.alloca i32 {bindc_name = "k"}
-! CHECK: %[[DC_J:.*]] = fir.alloca i32 {bindc_name = "j"}
-! CHECK: %[[DC_I:.*]] = fir.alloca i32 {bindc_name = "i"}
-! CHECK: %[[P_I:.*]] = acc.private varPtr(%[[DC_I]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "i"}
-! CHECK: %[[P_J:.*]] = acc.private varPtr(%[[DC_J]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "j"}
-! CHECK: %[[P_K:.*]] = acc.private varPtr(%[[DC_K]] : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "k"}
+! CHECK-SAME: %[[ARG_I:.*]]: !fir.ref<i32> {fir.bindc_name = "i"}
+! CHECK-SAME: %[[ARG_J:.*]]: !fir.ref<i32> {fir.bindc_name = "j"}
+! CHECK-SAME: %[[ARG_K:.*]]: !fir.ref<i32> {fir.bindc_name = "k"}
+! CHECK: %[[DC_I:.*]]:2 = hlfir.declare %[[ARG_I]] dummy_scope %0
+! CHECK: %[[DC_J:.*]]:2 = hlfir.declare %[[ARG_J]] dummy_scope %0 
+! CHECK: %[[DC_K:.*]]:2 = hlfir.declare %[[ARG_K]] dummy_scope %0 
+! CHECK: acc.parallel combined(loop)
+! CHECK: %[[P_I:.*]] = acc.private varPtr(%[[DC_I]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "i"}
+! CHECK: %[[P_J:.*]] = acc.private varPtr(%[[DC_J]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "j"}
+! CHECK: %[[P_K:.*]] = acc.private varPtr(%[[DC_K]]#0 : !fir.ref<i32>) -> !fir.ref<i32> {implicit = true, name = "k"}
 ! CHECK: acc.loop combined(parallel) private(@privatization_ref_i32 -> %[[P_I]] : !fir.ref<i32>, @privatization_ref_i32 -> %[[P_J]] : !fir.ref<i32>, @privatization_ref_i32 -> %[[P_K]] : !fir.ref<i32>) control(%{{.*}} : i32, %{{.*}} : i32, %{{.*}} : i32) = (%c1{{.*}}, %c1{{.*}}, %c1{{.*}} : i32, i32, i32) to (%c10{{.*}}, %c100{{.*}}, %c200{{.*}} : i32, i32, i32)  step (%c1{{.*}}, %c1{{.*}}, %c1{{.*}} : i32, i32, i32)
 ! CHECK: } attributes {inclusiveUpperbound = array<i1: true, true, true>, independent = [#acc.device_type<none>]}
diff --git a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90 b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
index 29985a02eb6ef..cfe27e4f8fca1 100644
--- a/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
+++ b/flang/test/Semantics/OpenACC/acc-kernels-loop.f90
@@ -243,8 +243,15 @@ program openacc_kernels_loop_validity
     a(i) = 3.14
   end do
 
+  !$acc kernels loop default(none) private(N, a)
+  do i = 1, N
+    a(i) = 3.14
+  end do
+
   !$acc kernels loop default(none)
+  !ERROR: The DEFAULT(NONE) clause requires that 'n' must be listed in a data-mapping clause
   do i = 1, N
+    !ERROR: The DEFAULT(NONE) clause requires that 'a' must be listed in a data-mapping clause
     a(i) = 3.14
   end do

clementval
clementval approved these changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

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

LGTM! I think the change in the lowering is fine and it's probably because we were not retrieving the value the same way in the symMap. Going through hlfir.declare seems right.

@razvanlupusoru
Copy link
Contributor

razvanlupusoru commented Jul 16, 2025

LGTM! I think the change in the lowering is fine and it's probably because we were not retrieving the value the same way in the symMap. Going through hlfir.declare seems right.

I agree.

razvanlupusoru
razvanlupusoru approved these changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@razvanlupusoru razvanlupusoru 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!

@akuhlens akuhlens merged commit abdd453 into llvm:main Jul 18, 2025
17 checks passed
This was referenced Jul 23, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@razvanlupusoru razvanlupusoru razvanlupusoru approved these changes

@clementval clementval clementval approved these changes

@klausler klausler klausler approved these changes

Assignees

No one assigned

Labels

flang:fir-hlfir flang:openmp flang:semantics 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.

5 participants

@akuhlens @llvmbot @razvanlupusoru @clementval @klausler