[Flang][OpenMP] Fix 'Internal: no symbol found' for OpenMP aligned and linear clause.

[sameeranjoshi]

Summary

The initial approach was to go with changing parser nodes from std::list<parser::Name> to OmpObjectList, but that might have lead to illegal programs.
Resolving the symbols inside OmpAttributeVisitor.
Fix a couple of XFAIL tests.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

357

So in the end you didn't need resolve-names and an easy resolve name inside resolve directive was doable...

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

357

Yes :), hopefully when I debugged how the actual resolution occurs.
But in later patches we might end up with needless repetition of code for fortran and OMP/OACC.

[kiranchandramohan]

Thanks @sameeranjoshi for this patch. Couple of questions inline.

flang/lib/Semantics/resolve-directives.cpp

323

Why does aligned require a new symbol?

974

Should we call AddToContextObjectWithDSA only for those flags which correspond to DSA (default, shared, private, lastprivate, firstprivate, linear)?

[sameeranjoshi]

Address review comments.

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

323

Looks like I was in assumption that we need the same flags from flang/include/flang/Semantics/symbol.h.

I have removed it.

974

Thanks for making me aware of that.
I have added a guard.

[kiranchandramohan]

Two more questions.

flang/lib/Semantics/resolve-directives.cpp

285–306

Would one of the following work and would it be better?

->

bool Pre(const parser::OmpLinearClause::WithoutModifier &x) {
  ResolveOmpNameList(x.names, Symbol::Flag::OmpLinear);
  return false;
}
bool Pre(const parser::OmpLinearClause::WithModifier &x) {
  ResolveOmpNameList(x.names, Symbol::Flag::OmpLinear);
  return false;
}

->

bool Pre(const parser::OmpLinearClause &x) {
  std::visit(common::visitors{
                 [&](const parser::OmpLinearClause::WithoutModifier
                         &linearWithoutModifier) {
                   ResolveOmpNameList(linearWithoutModifier.names, Symbol::Flag::OmpLinear);
                 },
                 [&](const parser::OmpLinearClause::WithModifier
                         &linearWithModifier) {
                   ResolveOmpNameList(linearWithModifier.names, Symbol::Flag::OmpLinear);

                 },
             },
      x.u);
  return false;
}

301

Can this be false here?

[sameeranjoshi]

Address review comments.

[sameeranjoshi]

Thanks for reviewing.

flang/lib/Semantics/resolve-directives.cpp

301

Keeping it for consistency sake with the convention followed in the file.
Just asking if you know is there some reason to use some specific boolean value?

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

301

See the following links for the general answer. Basically the walk wont proceed to the nested components and will not call Post if false is returned.

https://github.com/llvm/llvm-project/blob/32897c05ab6ba90c3496270046aa5664b37f6557/flang/include/flang/Parser/parse-tree-visitor.h#L23

https://github.com/llvm/llvm-project/blob/32897c05ab6ba90c3496270046aa5664b37f6557/flang/include/flang/Parser/parse-tree-visitor.h#L178

[clementval]

flang/lib/Semantics/resolve-directives.cpp

301

If you raise an error here you might want to return false to not go further in the visit since you might not have the information to continue in the nested nodes.

[sameeranjoshi]

Thank you for detailed information.
I missed a test which I realized when I was trying to rebase on master.
https://github.com/llvm/llvm-project/blob/f1a96de1bc8db527b5eb820c36c17e275900ca2b/flang/test/Semantics/omp-declarative-directive.f90#L14

I debugged to find that the aligned node is not visited for the above test, is !$omp declare missing here to visit ?

[clementval]

I debugged to find that the aligned node is not visited for the above test, is !$omp declare missing here to visit ?

Looks like declarative construct are visited from here in the OpenMP name resolution:
https://github.com/llvm/llvm-project/blob/f1a96de1bc8db527b5eb820c36c17e275900ca2b/flang/lib/Semantics/resolve-directives.cpp#L225

[sameeranjoshi]

After the patch D89385 there were regressions in this review before merging
it, probably due to bool Pre(const parser::SpecificationPart &x) was made to
return true hence the nodes under it were visited.
And the tests consisted of one such case which was causing fatal internal error: CHECK(!ompContext_.empty()).
Resolve symbols which were not found in OpenMPDeclareSimdConstruct to make this patch working.

[kiranktp]

LGTM

flang/include/flang/Parser/parse-tree.h

struct OmpIfClause {
   ENUM_CLASS(DirectiveNameModifier, Parallel, Target, TargetEnterData,
       TargetExitData, TargetData, TargetUpdate, Taskloop, Task)
   std::tuple<std::optional<DirectiveNameModifier>, ScalarLogicalExpr> t;
 };
 
 // 2.8.1 aligned-clause -> ALIGNED (variable-name-list[ : scalar-constant])
 struct OmpAlignedClause {
   TUPLE_CLASS_BOILERPLATE(OmpAlignedClause);
+  CharBlock source;
   std::tuple<std::list<Name>, std::optional<ScalarIntConstantExpr>> t;
 };
 
 // 2.15.3.7 linear-modifier -> REF | VAL | UVAL
 struct OmpLinearModifier {
   ENUM_CLASS(Type, Ref, Val, Uval)
   WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Type);
 };

flang/include/flang/Semantics/symbol.h

ENUM_CLASS(Flag,
       OmpShared, OmpPrivate, OmpLinear, OmpFirstPrivate, OmpLastPrivate,
       // OpenMP data-mapping attribute
       OmpMapTo, OmpMapFrom, OmpMapAlloc, OmpMapRelease, OmpMapDelete,
       // OpenMP data-copying attribute
       OmpCopyIn,
       // OpenMP miscellaneous flags
       OmpCommonBlock, OmpReduction, OmpAllocate, OmpDeclareSimd,
       OmpDeclareTarget, OmpThreadprivate, OmpDeclareReduction, OmpFlushed,
-      OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined);
+      OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined, OmpAligned);
   using Flags = common::EnumSet<Flag, Flag_enumSize>;
 
   const Scope &owner() const { return *owner_; }
   const SourceName &name() const { return name_; }
   Attrs &attrs() { return attrs_; }
   const Attrs &attrs() const { return attrs_; }
   Flags &flags() { return flags_; }
   const Flags &flags() const { return flags_; }

flang/lib/Semantics/resolve-directives.cpp

public:
   void Post(const parser::OmpBeginLoopDirective &) {
     GetContext().withinConstruct = true;
   }
   bool Pre(const parser::DoConstruct &);
 
   bool Pre(const parser::OpenMPSectionsConstruct &);
   void Post(const parser::OpenMPSectionsConstruct &) { PopContext(); }
 
+  bool Pre(const parser::OpenMPDeclareSimdConstruct &x) {
+    PushContext(x.source, llvm::omp::Directive::OMPD_declare_simd);
+    const auto &name{std::get<std::optional<parser::Name>>(x.t)};
+    if (name) {
+      ResolveOmpName(*name, Symbol::Flag::OmpDeclareSimd);
+    }
+    return true;
+  }
+  void Post(const parser::OpenMPDeclareSimdConstruct &) { PopContext(); }
   bool Pre(const parser::OpenMPThreadprivate &);
   void Post(const parser::OpenMPThreadprivate &) { PopContext(); }
 
   // 2.15.3 Data-Sharing Attribute Clauses
   void Post(const parser::OmpDefaultClause &);
   bool Pre(const parser::OmpClause::Shared &x) {
     ResolveOmpObjectList(x.v, Symbol::Flag::OmpShared);
     return false;
public:
   bool Pre(const parser::OmpClause::Lastprivate &x) {
     ResolveOmpObjectList(x.v, Symbol::Flag::OmpLastPrivate);
     return false;
   }
   bool Pre(const parser::OmpClause::Copyin &x) {
     ResolveOmpObjectList(x.v, Symbol::Flag::OmpCopyIn);
     return false;
   }
-
+  bool Pre(const parser::OmpLinearClause &x) {
+    std::visit(common::visitors{
+                   [&](const parser::OmpLinearClause::WithoutModifier
+                           &linearWithoutModifier) {
+                     ResolveOmpNameList(
+                         linearWithoutModifier.names, Symbol::Flag::OmpLinear);
+                   },
+                   [&](const parser::OmpLinearClause::WithModifier
+                           &linearWithModifier) {
+                     ResolveOmpNameList(
+                         linearWithModifier.names, Symbol::Flag::OmpLinear);
+                   },
+               },
+        x.u);
+    return false;
+  }
+  bool Pre(const parser::OmpAlignedClause &x) {
+    const auto &alignedNameList{std::get<std::list<parser::Name>>(x.t)};
+    ResolveOmpNameList(alignedNameList, Symbol::Flag::OmpAligned);
+    return false;
+  }
   void Post(const parser::Name &);
 
 private:
   std::int64_t GetAssociatedLoopLevelFromClauses(const parser::OmpClauseList &);
 
   static constexpr Symbol::Flags dataSharingAttributeFlags{
       Symbol::Flag::OmpShared, Symbol::Flag::OmpPrivate,
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
       Symbol::Flag::OmpReduction, Symbol::Flag::OmpLinear};
 
   static constexpr Symbol::Flags privateDataSharingAttributeFlags{
       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpFirstPrivate,
       Symbol::Flag::OmpLastPrivate};
 
   static constexpr Symbol::Flags ompFlagsRequireNewSymbol{
       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpLinear,
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
       Symbol::Flag::OmpReduction};
 
   static constexpr Symbol::Flags ompFlagsRequireMark{
       Symbol::Flag::OmpThreadprivate};
 
   static constexpr Symbol::Flags dataCopyingAttributeFlags{
       Symbol::Flag::OmpCopyIn};
 
   std::vector<const parser::Name *> allocateNames_; // on one directive
private:
   void PrivatizeAssociatedLoopIndex(const parser::OpenMPLoopConstruct &);
   void ResolveSeqLoopIndexInParallelOrTaskConstruct(const parser::Name &);
 
   void ResolveOmpObjectList(const parser::OmpObjectList &, Symbol::Flag);
   void ResolveOmpObject(const parser::OmpObject &, Symbol::Flag);
   Symbol *ResolveOmp(const parser::Name &, Symbol::Flag, Scope &);
   Symbol *ResolveOmp(Symbol &, Symbol::Flag, Scope &);
   Symbol *ResolveOmpCommonBlockName(const parser::Name *);
+  void ResolveOmpNameList(const std::list<parser::Name> &, Symbol::Flag);
+  void ResolveOmpName(const parser::Name &, Symbol::Flag);
+  Symbol *ResolveName(const parser::Name *);
   Symbol *DeclareOrMarkOtherAccessEntity(const parser::Name &, Symbol::Flag);
   Symbol *DeclareOrMarkOtherAccessEntity(Symbol &, Symbol::Flag);
   void CheckMultipleAppearances(
       const parser::Name &, const Symbol &, Symbol::Flag);
 
   void CheckDataCopyingClause(
       const parser::Name &, const Symbol &, Symbol::Flag);
 };
if (!symbol->owner().IsDerivedType() && !symbol->has<ProcEntityDetails>() &&
               "a data-sharing attribute clause"_err_en_US,
               symbol->name());
         }
       }
     }
   } // within OpenMP construct
 }
 
+Symbol *OmpAttributeVisitor::ResolveName(const parser::Name *name) {
+  if (auto *resolvedSymbol{
+          name ? GetContext().scope.FindSymbol(name->source) : nullptr}) {
+    name->symbol = resolvedSymbol;
+    return resolvedSymbol;
+  } else {
+    return nullptr;
+  }
+}
+
+void OmpAttributeVisitor::ResolveOmpName(
+    const parser::Name &name, Symbol::Flag ompFlag) {
+  if (ResolveName(&name)) {
+    if (auto *resolvedSymbol{ResolveOmp(name, ompFlag, currScope())}) {
+      if (dataSharingAttributeFlags.test(ompFlag)) {
+        AddToContextObjectWithDSA(*resolvedSymbol, ompFlag);
+      }
+    }
+  }
+}
+
+void OmpAttributeVisitor::ResolveOmpNameList(
+    const std::list<parser::Name> &nameList, Symbol::Flag ompFlag) {
+  for (const auto &name : nameList) {
+    ResolveOmpName(name, ompFlag);
+  }
+}
+
 Symbol *OmpAttributeVisitor::ResolveOmpCommonBlockName(
     const parser::Name *name) {
   if (auto *prev{name
               ? GetContext().scope.parent().FindCommonBlock(name->source)
               : nullptr}) {
     name->symbol = prev;
     return prev;
   }

flang/test/Semantics/omp-clause-validity01.f90

!                     ordered-clause
   !ERROR: At most one SCHEDULE clause can appear on the DO directive
   !ERROR: When SCHEDULE clause has RUNTIME specified, it must not have chunk size specified
   !$omp do schedule(auto, 2) schedule(runtime, 2)
   do i = 1, N
      a = 3.14
   enddo
 
   !ERROR: A modifier may not be specified in a LINEAR clause on the DO directive
-  !ERROR: Internal: no symbol found for 'b'
   !$omp do linear(ref(b))
   do i = 1, N
      a = 3.14
   enddo
 
   !ERROR: The NONMONOTONIC modifier can only be specified with SCHEDULE(DYNAMIC) or SCHEDULE(GUIDED)
   !ERROR: The NONMONOTONIC modifier cannot be specified if an ORDERED clause is specified
   !$omp do schedule(NONMONOTONIC:static) ordered
   do i = 1, N
      a = 3.14
   enddo
 
   !$omp do schedule(simd, monotonic:dynamic)
   do i = 1, N
      a = 3.14
   enddo
 
   !ERROR: The parameter of the ORDERED clause must be a constant positive integer expression
   !ERROR: A loop directive may not have both a LINEAR clause and an ORDERED clause with a parameter
-  !ERROR: Internal: no symbol found for 'b'
-  !ERROR: Internal: no symbol found for 'a'
   !$omp do ordered(1-1) private(b) linear(b) linear(a)
   do i = 1, N
      a = 3.14
   enddo
 
   !ERROR: The parameter of the ORDERED clause must be greater than or equal to the parameter of the COLLAPSE clause
   !$omp do collapse(num-14) ordered(1)
   do i = 1, N
!                      collapse-clause
 
   !ERROR: The parameter of the SIMDLEN clause must be a constant positive integer expression
   !$omp simd simdlen(-1)
   do i = 1, N
      a = 3.14
   enddo
 
   !ERROR: The ALIGNMENT parameter of the ALIGNED clause must be a constant positive integer expression
-  !ERROR: Internal: no symbol found for 'b'
   !$omp simd aligned(b:-2)
   do i = 1, N
      a = 3.14
   enddo
 
   !$omp parallel
   !ERROR: The parameter of the SIMDLEN clause must be less than or equal to the parameter of the SAFELEN clause
   !$omp simd safelen(1+1) simdlen(1+2)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end parallel
 
 ! 2.11.1 parallel-do-clause -> parallel-clause |
 !                              do-clause
 
   !ERROR: At most one PROC_BIND clause can appear on the PARALLEL DO directive
   !ERROR: A modifier may not be specified in a LINEAR clause on the PARALLEL DO directive
-  !ERROR: Internal: no symbol found for 'b'
   !$omp parallel do proc_bind(master) proc_bind(close) linear(val(b))
   do i = 1, N
      a = 3.14
   enddo
 
 ! 2.8.3 do-simd-clause -> do-clause |
 !                         simd-clause
 
!                               simd-clause
   !ERROR: GRAINSIZE and NUM_TASKS clauses are mutually exclusive and may not appear on the same TASKLOOP SIMD directive
   !$omp taskloop simd num_tasks(3) grainsize(2)
   do i = 1,N
      a = 3.14
   enddo
 
   !ERROR: The parameter of the SIMDLEN clause must be a constant positive integer expression
   !ERROR: The ALIGNMENT parameter of the ALIGNED clause must be a constant positive integer expression
-  !ERROR: Internal: no symbol found for 'a'
   !$omp taskloop simd simdlen(-1) aligned(a:-2)
   do i = 1, N
      a = 3.14
   enddo
 end program

flang/test/Semantics/omp-declarative-directive.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
 
 ! Check OpenMP declarative directives
 
 !TODO: all internal errors
 !      enable declare-reduction example after name resolution
 
 ! 2.8.2 declare-simd
 
 subroutine declare_simd_1(a, b)
   real(8), intent(inout) :: a, b
-  !ERROR: Internal: no symbol found for 'declare_simd_1'
-  !ERROR: Internal: no symbol found for 'a'
   !$omp declare simd(declare_simd_1) aligned(a)
   a = 3.14 + b
 end subroutine declare_simd_1
 
 module m1
   abstract interface
      subroutine sub(x,y)
        integer, intent(in)::x
        integer, intent(in)::y
      end subroutine sub
   end interface
 end module m1
 
 subroutine declare_simd_2
   use m1
   procedure (sub) sub1
-  !ERROR: Internal: no symbol found for 'sub1'
   !ERROR: NOTINBRANCH and INBRANCH clauses are mutually exclusive and may not appear on the same DECLARE SIMD directive
   !$omp declare simd(sub1) inbranch notinbranch
   procedure (sub), pointer::p
   p=>sub1
   call p(5,10)
 end subroutine declare_simd_2
 
 subroutine sub1 (x,y)

flang/test/Semantics/omp-do03.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.7.1 Loop Construct
 ! Semantic error for correct test case
 
 program omp_do
   integer i, j, k
   integer :: a(10), b(10)

flang/test/Semantics/omp-loop-simd01.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.8.3 Loop simd Construct
 ! Semantic error for correct test case.
 
 program omp_loop_simd
   integer i, j, k, l
   k = 0;

flang/test/Semantics/omp-simd02.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.8.1 simd Construct
 ! Semantic error for correct test case
 
 program omp_simd
   integer i, j, k
   integer, allocatable :: a(:)