[flang-commits] [flang] [flang][openacc] Avoid privatizing symbols during semantics (PR #69506)

[via flang-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-openacc

Author: Razvan Lupusoru (razvanlupusoru)

<details>
<summary>Changes</summary>

During flang handling of semantics of OpenACC private/firstprivate/ reduction clauses (including the implicitly private loop IV), a new scoped symbol was being created. This could lead to ambiguity in the lowered FIR - aka having multiple fir.declare for the same symbol. Because lowering of OpenACC does not materialize the meaning of the private clauses (by actually creating a scoped local symbol), it does not make sense to create a new symbol in semantics either.

I updated the acc-symbols01.f90 test to reflect this updated approach. Technically, the test could be removed, but it made sense to keep in place to highlight this intentional decision.

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


2 Files Affected:

- (modified) flang/lib/Semantics/resolve-directives.cpp (+8-33) 
- (modified) flang/test/Semantics/OpenACC/acc-symbols01.f90 (+4-4) 


``````````diff
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 7c8fdb651af9fdd..e8448a36a7b273c 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -254,9 +254,6 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
       Symbol::Flag::AccCopyIn, Symbol::Flag::AccCopyOut,
       Symbol::Flag::AccDelete, Symbol::Flag::AccPresent};
 
-  Symbol::Flags accFlagsRequireNewSymbol{Symbol::Flag::AccPrivate,
-      Symbol::Flag::AccFirstPrivate, Symbol::Flag::AccReduction};
-
   Symbol::Flags accDataMvtFlags{
       Symbol::Flag::AccDevice, Symbol::Flag::AccHost, Symbol::Flag::AccSelf};
 
@@ -266,7 +263,7 @@ class AccAttributeVisitor : DirectiveAttributeVisitor<llvm::acc::Directive> {
       Symbol::Flag::AccDevicePtr, Symbol::Flag::AccDeviceResident,
       Symbol::Flag::AccLink, Symbol::Flag::AccPresent};
 
-  void PrivatizeAssociatedLoopIndex(const parser::OpenACCLoopConstruct &);
+  void CheckAssociatedLoopIndex(const parser::OpenACCLoopConstruct &);
   void ResolveAccObjectList(const parser::AccObjectList &, Symbol::Flag);
   void ResolveAccObject(const parser::AccObject &, Symbol::Flag);
   Symbol *ResolveAcc(const parser::Name &, Symbol::Flag, Scope &);
@@ -877,7 +874,7 @@ bool AccAttributeVisitor::Pre(const parser::OpenACCLoopConstruct &x) {
   }
   ClearDataSharingAttributeObjects();
   SetContextAssociatedLoopLevel(GetAssociatedLoopLevelFromClauses(clauseList));
-  PrivatizeAssociatedLoopIndex(x);
+  CheckAssociatedLoopIndex(x);
   return true;
 }
 
@@ -1141,13 +1138,12 @@ std::int64_t AccAttributeVisitor::GetAssociatedLoopLevelFromClauses(
   return 1; // default is outermost loop
 }
 
-void AccAttributeVisitor::PrivatizeAssociatedLoopIndex(
+void AccAttributeVisitor::CheckAssociatedLoopIndex(
     const parser::OpenACCLoopConstruct &x) {
   std::int64_t level{GetContext().associatedLoopLevel};
-  if (level <= 0) { // collpase value was negative or 0
+  if (level <= 0) { // collapse value was negative or 0
     return;
   }
-  Symbol::Flag ivDSA{Symbol::Flag::AccPrivate};
 
   const auto getNextDoConstruct =
       [this](const parser::Block &block) -> const parser::DoConstruct * {
@@ -1166,16 +1162,8 @@ void AccAttributeVisitor::PrivatizeAssociatedLoopIndex(
 
   const auto &outer{std::get<std::optional<parser::DoConstruct>>(x.t)};
   for (const parser::DoConstruct *loop{&*outer}; loop && level > 0; --level) {
-    // go through all the nested do-loops and resolve index variables
-    const parser::Name *iv{GetLoopIndex(*loop)};
-    if (iv) {
-      if (auto *symbol{ResolveAcc(*iv, ivDSA, currScope())}) {
-        symbol->set(Symbol::Flag::AccPreDetermined);
-        iv->symbol = symbol; // adjust the symbol within region
-        AddToContextObjectWithDSA(*symbol, ivDSA);
-      }
-    }
-
+    // Go through all nested loops to ensure index variable exists.
+    GetLoopIndex(*loop);
     const auto &block{std::get<parser::Block>(loop->t)};
     loop = getNextDoConstruct(block);
   }
@@ -1328,20 +1316,12 @@ void AccAttributeVisitor::ResolveAccObject(
 
 Symbol *AccAttributeVisitor::ResolveAcc(
     const parser::Name &name, Symbol::Flag accFlag, Scope &scope) {
-  if (accFlagsRequireNewSymbol.test(accFlag)) {
-    return DeclarePrivateAccessEntity(name, accFlag, scope);
-  } else {
-    return DeclareOrMarkOtherAccessEntity(name, accFlag);
-  }
+  return DeclareOrMarkOtherAccessEntity(name, accFlag);
 }
 
 Symbol *AccAttributeVisitor::ResolveAcc(
     Symbol &symbol, Symbol::Flag accFlag, Scope &scope) {
-  if (accFlagsRequireNewSymbol.test(accFlag)) {
-    return DeclarePrivateAccessEntity(symbol, accFlag, scope);
-  } else {
-    return DeclareOrMarkOtherAccessEntity(symbol, accFlag);
-  }
+  return DeclareOrMarkOtherAccessEntity(symbol, accFlag);
 }
 
 Symbol *AccAttributeVisitor::DeclareOrMarkOtherAccessEntity(
@@ -1374,11 +1354,6 @@ static bool WithMultipleAppearancesAccException(
 void AccAttributeVisitor::CheckMultipleAppearances(
     const parser::Name &name, const Symbol &symbol, Symbol::Flag accFlag) {
   const auto *target{&symbol};
-  if (accFlagsRequireNewSymbol.test(accFlag)) {
-    if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
-      target = &details->symbol();
-    }
-  }
   if (HasDataSharingAttributeObject(*target) &&
       !WithMultipleAppearancesAccException(symbol, accFlag)) {
     context_.Say(name.source,
diff --git a/flang/test/Semantics/OpenACC/acc-symbols01.f90 b/flang/test/Semantics/OpenACC/acc-symbols01.f90
index ddb87711eecc5e0..375445bad13a54f 100644
--- a/flang/test/Semantics/OpenACC/acc-symbols01.f90
+++ b/flang/test/Semantics/OpenACC/acc-symbols01.f90
@@ -14,11 +14,11 @@ program mm
   b = 2
  !$acc parallel present(c) firstprivate(b) private(a)
  !$acc loop
-  !DEF: /mm/OtherConstruct1/i (AccPrivate, AccPreDetermined) HostAssoc INTEGER(4)
+  !REF: /mm/i
   do i=1,10
-   !DEF: /mm/OtherConstruct1/a (AccPrivate) HostAssoc INTEGER(4)
-   !REF: /mm/OtherConstruct1/i
-   !DEF: /mm/OtherConstruct1/b (AccFirstPrivate) HostAssoc INTEGER(4)
+   !REF: /mm/a
+   !REF: /mm/i
+   !REF: /mm/b
    a(i) = b(i)
   end do
  !$acc end parallel

``````````

</details>


https://github.com/llvm/llvm-project/pull/69506