[OpenACC] Switch Clang to use the Flang 'appertainment' rules for cla… (llvm#135372)

…uses

The Flang implemenation of OpenACC uses a .td file in the llvm/Frontend
directory to determine appertainment in 4 categories:

-Required: If this list has items in it, the directive requires at least
1 of these be present.

-AllowedExclusive: Items on this list are all allowed, but only 1 from
the list may be here (That is, they are exclusive of eachother).

-AllowedOnce: Items on this list are all allowed, but may not be
duplicated.

Allowed: Items on this list are allowed. Note th at the actual list of
'allowed' is all 4 of these lists together.

This is a draft patch to swtich Clang over to use those tables. Surgery
to get this to happen in Clang Sema was somewhat reasonable. However,
some gaps in the implementations are obvious, the existing clang
implementation disagrees with the Flang interpretation of it. SO, we're
keeping a task list here based on what gets discovered.

Changes to Clang:
- [x] Switch 'directive-kind' enum conversions to use tablegen See
ff1a7bd
- [x] Switch 'clause-kind' enum conversions to use tablegen See
ff1a7bd
- [x] Investigate 'parse' test differences to see if any new
disagreements arise.
- [x] Clang/Flang disagree as to whether 'collapse' can be multiple
times on a loop. Further research showed no prose to limit this, and the
comment on the clang implementation said "no good reason to allow", so
no standards justification.
- [x] Clang/Flang disagree whether 'num_gangs' can appear >1 on a
compute/combined construct. This ended up being an unjustified
restriction.
- [x] Clang/Flang disagree as to the list of required clauses on a 'set'
construct. My research shows that Clang mistakenly included 'if' in the
list, and that it should be just 'default_async', 'device_num', and
'device_type'.
- [x] Order of 'at least one of' diagnostic has changed. Tests were
updated.
- [x] Ensure we are properly 'de-aliasing' clause names in appertainment
checks?
- [x] What is 'shortloop'? 'shortloop' seems to be an old non-standard
extension that isn't supported by flang, but is parsed for backward
compat reasons. Clang won't parse, but we at least have a spot for it in
the clause list.
- [x] Implemented proposed change for 'routine' gang/worker/vector/seq.
(see issue 539)
- [x] Implement init/shutdown can only have 1 'if' (see issue 540)
- [x] Clang/Flang disagree as to whether 'tile' is permitted more than
once on a 'loop' or combined constructs (Flang prohibits >1). I see no
justification for this in the standard. EDIT: I found a comment in clang
that I did this to make SOMETHING around duplicate checks easier.
Discussion showed we should actually have a better behavior around
'device_type' and duplicates, so I've since implemented that.
- [x] Clang/Flang disagree whether 'gang', 'worker', or 'vector' may
appear on the same construct as a 'seq' on a 'loop' or 'combined'. There
is prose for this in 2022: (a gang, worker, or vector clause may not
appear if a 'seq' clause appears). EDIT: These don't actually disagree,
but aren't in the .td file, so I restored the existing code to do this.
- [x] Clang/Flang disagree on whether 'bind' can appear >1 on a
'routine'. I believe line 3096 (A bind clause may not bind to a routine
name that has a visible bind clause) makes this limitation (Flang
permits >1 bind). we discussed and decided this should have the same
rules as worker/vector/etc, except without the 'exactly 1 of' rule (so
no dupes in individual sections).
- [x] Clang/Flang disagree on whether 'init'/'shutdown' can have
multiple 'device_num' clauses. I believe there is no supporting prose
for this limitation., We decided that `device_num` should only happen
1x.
- [x] Clang/Flang disagree whether 'num_gangs' can appear >1 on a
'kernels' construct. Line 1173 (On a kernels construct, the num_gangs
clause must have a single argument) justifies limiting on a
per-arguement basis, but doesn't do so for multiple num_gangs clauses.
WE decided to do this with the '1-per-device-type' region for num_gangs,
num_workers, and vector_length, see openacc bug here:
https://github.com/OpenACC/openacc-spec/issues/541

Changes to Flang:
- [x] Clang/Flang disgree on whether 'atomic' can take an 'if' clause.
This was added in OpenACC3.3_Next See llvm#135451
- [x] Clang/Flang disagree on whether 'finalize' can be allowed >1 times
on a 'exit_data' construct. see llvm#135415.
- [x] Clang/Flang disagree whether 'if_present' should be allowed >1
times on a 'host_data'/'update' construct. see llvm#135422
- [x] Clang/Flang disagree on whether 'init'/'shutdown' can have
multiple 'device_type' clauses. I believe there is no supporting prose
for this limitation.
- [ ] SEE change for num_gangs/etc above.


Changes that need discussion/research:

Author: erichkeane

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -12961,6 +12961,19 @@ def err_acc_clause_after_device_type
 def err_acc_clause_cannot_combine
     : Error<"OpenACC clause '%0' may not appear on the same construct as a "
             "'%1' clause on a '%2' construct">;
+def err_acc_clause_routine_cannot_combine_before_device_type
+    : Error<"OpenACC clause '%0' after 'device_type' clause on a 'routine' "
+              "conflicts with the '%1' clause before the first 'device_type'">;
+def err_acc_clause_routine_cannot_combine_same_device_type
+    : Error<"OpenACC clause '%0' on a 'routine' directive conflicts with the "
+            "'%1' clause applying to the same 'device_type'">;
+def err_acc_clause_routine_one_of_in_region
+    : Error<"OpenACC 'routine' construct must have at least one 'gang', 'seq', "
+            "'vector', or 'worker' clause that applies to each 'device_type'">;
+def err_acc_clause_since_last_device_type
+    : Error<"OpenACC '%0' clause cannot appear more than once%select{| in a "
+            "'device_type' region}2 on a '%1' directive">;
+
 def err_acc_reduction_num_gangs_conflict
     : Error<"OpenACC '%1' clause %select{|with more than 1 argument }0may not "
             "appear on a '%2' construct "
@@ -12978,9 +12991,6 @@ def note_acc_reduction_composite_member_loc : Note<"invalid field is here">;
 def err_acc_loop_not_for_loop
     : Error<"OpenACC '%0' construct can only be applied to a 'for' loop">;
 def note_acc_construct_here : Note<"'%0' construct is here">;
-def err_acc_loop_spec_conflict
-    : Error<"OpenACC clause '%0' on '%1' construct conflicts with previous "
-            "data dependence clause">;
 def err_acc_collapse_loop_count
     : Error<"OpenACC 'collapse' clause loop count must be a %select{constant "
             "expression|positive integer value, evaluated to %1}0">;
@@ -13032,9 +13042,10 @@ def err_acc_gang_reduction_numgangs_conflict
             "'%1' clause %select{inside a compute construct with a|and a}3 "
             "'num_gangs' clause with more than one argument">;
 
-    def err_reduction_op_mismatch
+def err_reduction_op_mismatch
     : Error<"OpenACC 'reduction' variable must have the same operator in all "
             "nested constructs (%0 vs %1)">;
+
 def err_acc_loop_variable_type
     : Error<"loop variable of loop associated with an OpenACC '%0' construct "
             "must be of integer, pointer, or random-access-iterator type (is "

clang/include/clang/Basic/OpenACCKinds.h

@@ -202,133 +202,140 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &Out,
   return printOpenACCAtomicKind(Out, AK);
 }
 
-/// Represents the kind of an OpenACC clause.
+/// Represents the kind of an OpenACC clause. Sorted alphabetically, since this
+/// order ends up influencing the sorting of the list diagnostic.
 enum class OpenACCClauseKind : uint8_t {
-  /// 'finalize' clause, allowed on 'exit data' directive.
-  Finalize,
-  /// 'if_present' clause, allowed on 'host_data' and 'update' directives.
-  IfPresent,
-  /// 'seq' clause, allowed on 'loop' and 'routine' directives.
-  Seq,
-  /// 'independent' clause, allowed on 'loop' directives.
-  Independent,
+  /// 'async' clause, allowed on Compute, Data, 'update', 'wait', and Combined
+  /// constructs.
+  Async,
+  /// 'attach' clause, allowed on Compute and Combined constructs, plus 'data'
+  /// and 'enter data'.
+  Attach,
   /// 'auto' clause, allowed on 'loop' directives.
   Auto,
-  /// 'worker' clause, allowed on 'loop', Combined, and 'routine' directives.
-  Worker,
-  /// 'vector' clause, allowed on 'loop', Combined, and 'routine' directives.
-  Vector,
-  /// 'nohost' clause, allowed on 'routine' directives.
-  NoHost,
-  /// 'default' clause, allowed on parallel, serial, kernel (and compound)
-  /// constructs.
-  Default,
-  /// 'if' clause, allowed on all the Compute Constructs, Data Constructs,
-  /// Executable Constructs, and Combined Constructs.
-  If,
-  /// 'self' clause, allowed on Compute and Combined Constructs, plus 'update'.
-  Self,
+  /// 'bind' clause, allowed on routine constructs.
+  Bind,
+  /// 'collapse' clause, allowed on 'loop' and Combined constructs.
+  Collapse,
   /// 'copy' clause, allowed on Compute and Combined Constructs, plus 'data' and
   /// 'declare'.
   Copy,
   /// 'copy' clause alias 'pcopy'.  Preserved for diagnostic purposes.
   PCopy,
   /// 'copy' clause alias 'present_or_copy'.  Preserved for diagnostic purposes.
   PresentOrCopy,
-  /// 'use_device' clause, allowed on 'host_data' construct.
-  UseDevice,
-  /// 'attach' clause, allowed on Compute and Combined constructs, plus 'data'
-  /// and 'enter data'.
-  Attach,
+  /// 'copyin' clause, allowed on Compute and Combined constructs, plus 'data',
+  /// 'enter data', and 'declare'.
+  CopyIn,
+  /// 'copyin' clause alias 'pcopyin'.  Preserved for diagnostic purposes.
+  PCopyIn,
+  /// 'copyin' clause alias 'present_or_copyin'.  Preserved for diagnostic
+  /// purposes.
+  PresentOrCopyIn,
+  /// 'copyout' clause, allowed on Compute and Combined constructs, plus 'data',
+  /// 'exit data', and 'declare'.
+  CopyOut,
+  /// 'copyout' clause alias 'pcopyout'.  Preserved for diagnostic purposes.
+  PCopyOut,
+  /// 'copyout' clause alias 'present_or_copyout'.  Preserved for diagnostic
+  /// purposes.
+  PresentOrCopyOut,
+  /// 'create' clause, allowed on Compute and Combined constructs, plus 'data',
+  /// 'enter data', and 'declare'.
+  Create,
+  /// 'create' clause alias 'pcreate'.  Preserved for diagnostic purposes.
+  PCreate,
+  /// 'create' clause alias 'present_or_create'.  Preserved for diagnostic
+  /// purposes.
+  PresentOrCreate,
+  /// 'default' clause, allowed on parallel, serial, kernel (and compound)
+  /// constructs.
+  Default,
+  /// 'default_async' clause, allowed on 'set' construct.
+  DefaultAsync,
   /// 'delete' clause, allowed on the 'exit data' construct.
   Delete,
   /// 'detach' clause, allowed on the 'exit data' construct.
   Detach,
   /// 'device' clause, allowed on the 'update' construct.
   Device,
+  /// 'device_num' clause, allowed on 'init', 'shutdown', and 'set' constructs.
+  DeviceNum,
   /// 'deviceptr' clause, allowed on Compute and Combined Constructs, plus
   /// 'data' and 'declare'.
   DevicePtr,
   /// 'device_resident' clause, allowed on the 'declare' construct.
   DeviceResident,
+  /// 'device_type' clause, allowed on Compute, 'data', 'init', 'shutdown',
+  /// 'set', update', 'loop', 'routine', and Combined constructs.
+  DeviceType,
+  /// 'dtype' clause, an alias for 'device_type', stored separately for
+  /// diagnostic purposes.
+  DType,
+  /// 'finalize' clause, allowed on 'exit data' directive.
+  Finalize,
   /// 'firstprivate' clause, allowed on 'parallel', 'serial', 'parallel loop',
   /// and 'serial loop' constructs.
   FirstPrivate,
+  /// 'gang' clause, allowed on 'loop' and Combined constructs.
+  Gang,
   /// 'host' clause, allowed on 'update' construct.
   Host,
+  /// 'if' clause, allowed on all the Compute Constructs, Data Constructs,
+  /// Executable Constructs, and Combined Constructs.
+  If,
+  /// 'if_present' clause, allowed on 'host_data' and 'update' directives.
+  IfPresent,
+  /// 'independent' clause, allowed on 'loop' directives.
+  Independent,
   /// 'link' clause, allowed on 'declare' construct.
   Link,
   /// 'no_create' clause, allowed on allowed on Compute and Combined constructs,
   /// plus 'data'.
   NoCreate,
+  /// 'nohost' clause, allowed on 'routine' directives.
+  NoHost,
+  /// 'num_gangs' clause, allowed on 'parallel', 'kernels', parallel loop', and
+  /// 'kernels loop' constructs.
+  NumGangs,
+  /// 'num_workers' clause, allowed on 'parallel', 'kernels', parallel loop',
+  /// and 'kernels loop' constructs.
+  NumWorkers,
   /// 'present' clause, allowed on Compute and Combined constructs, plus 'data'
   /// and 'declare'.
   Present,
   /// 'private' clause, allowed on 'parallel', 'serial', 'loop', 'parallel
   /// loop', and 'serial loop' constructs.
   Private,
-  /// 'copyout' clause, allowed on Compute and Combined constructs, plus 'data',
-  /// 'exit data', and 'declare'.
-  CopyOut,
-  /// 'copyout' clause alias 'pcopyout'.  Preserved for diagnostic purposes.
-  PCopyOut,
-  /// 'copyout' clause alias 'present_or_copyout'.  Preserved for diagnostic
-  /// purposes.
-  PresentOrCopyOut,
-  /// 'copyin' clause, allowed on Compute and Combined constructs, plus 'data',
-  /// 'enter data', and 'declare'.
-  CopyIn,
-  /// 'copyin' clause alias 'pcopyin'.  Preserved for diagnostic purposes.
-  PCopyIn,
-  /// 'copyin' clause alias 'present_or_copyin'.  Preserved for diagnostic
-  /// purposes.
-  PresentOrCopyIn,
-  /// 'create' clause, allowed on Compute and Combined constructs, plus 'data',
-  /// 'enter data', and 'declare'.
-  Create,
-  /// 'create' clause alias 'pcreate'.  Preserved for diagnostic purposes.
-  PCreate,
-  /// 'create' clause alias 'present_or_create'.  Preserved for diagnostic
-  /// purposes.
-  PresentOrCreate,
   /// 'reduction' clause, allowed on Parallel, Serial, Loop, and the combined
   /// constructs.
   Reduction,
-  /// 'collapse' clause, allowed on 'loop' and Combined constructs.
-  Collapse,
-  /// 'bind' clause, allowed on routine constructs.
-  Bind,
+  /// 'self' clause, allowed on Compute and Combined Constructs, plus 'update'.
+  Self,
+  /// 'seq' clause, allowed on 'loop' and 'routine' directives.
+  Seq,
+  /// 'tile' clause, allowed on 'loop' and Combined constructs.
+  Tile,
+  /// 'use_device' clause, allowed on 'host_data' construct.
+  UseDevice,
+  /// 'vector' clause, allowed on 'loop', Combined, and 'routine' directives.
+  Vector,
   /// 'vector_length' clause, allowed on 'parallel', 'kernels', 'parallel loop',
   /// and 'kernels loop' constructs.
   VectorLength,
-  /// 'num_gangs' clause, allowed on 'parallel', 'kernels', parallel loop', and
-  /// 'kernels loop' constructs.
-  NumGangs,
-  /// 'num_workers' clause, allowed on 'parallel', 'kernels', parallel loop',
-  /// and 'kernels loop' constructs.
-  NumWorkers,
-  /// 'device_num' clause, allowed on 'init', 'shutdown', and 'set' constructs.
-  DeviceNum,
-  /// 'default_async' clause, allowed on 'set' construct.
-  DefaultAsync,
-  /// 'device_type' clause, allowed on Compute, 'data', 'init', 'shutdown',
-  /// 'set', update', 'loop', 'routine', and Combined constructs.
-  DeviceType,
-  /// 'dtype' clause, an alias for 'device_type', stored separately for
-  /// diagnostic purposes.
-  DType,
-  /// 'async' clause, allowed on Compute, Data, 'update', 'wait', and Combined
-  /// constructs.
-  Async,
-  /// 'tile' clause, allowed on 'loop' and Combined constructs.
-  Tile,
-  /// 'gang' clause, allowed on 'loop' and Combined constructs.
-  Gang,
   /// 'wait' clause, allowed on Compute, Data, 'update', and Combined
   /// constructs.
   Wait,
+  /// 'worker' clause, allowed on 'loop', Combined, and 'routine' directives.
+  Worker,
 
-  /// Represents an invalid clause, for the purposes of parsing.
+  /// 'shortloop' is represented in the ACC.td file, but isn't present in the
+  /// standard. This appears to be an old extension for the nvidia fortran
+  // compiler, but seemingly not elsewhere. Put it here as a placeholder, but it
+  // is never expected to be generated.
+  Shortloop,
+  /// Represents an invalid clause, for the purposes of parsing. Should be
+  /// 'last'.
   Invalid,
 };
 
@@ -485,6 +492,9 @@ inline StreamTy &printOpenACCClauseKind(StreamTy &Out, OpenACCClauseKind K) {
   case OpenACCClauseKind::Wait:
     return Out << "wait";
 
+  case OpenACCClauseKind::Shortloop:
+    llvm_unreachable("Shortloop shouldn't be generated in clang");
+    LLVM_FALLTHROUGH;
   case OpenACCClauseKind::Invalid:
     return Out << "<invalid>";
   }

clang/include/clang/Sema/SemaOpenACC.h

@@ -78,7 +78,7 @@ class SemaOpenACC : public SemaBase {
   /// Collapse has an 'N' count that makes it apply to a number of loops 'below'
   /// it.
   struct CollapseCheckingInfo {
-    OpenACCCollapseClause *ActiveCollapse = nullptr;
+    const OpenACCCollapseClause *ActiveCollapse = nullptr;
 
     /// This is a value that maintains the current value of the 'N' on the
     /// current collapse, minus the depth that has already been traversed. When
@@ -107,7 +107,7 @@ class SemaOpenACC : public SemaBase {
     /// own counting of elements.
     UnsignedOrNone CurTileCount = std::nullopt;
 
-    /// Records whether we've hit a 'CurTileCount' of '0' on the wya down,
+    /// Records whether we've hit a 'CurTileCount' of '0' on the way down,
     /// which allows us to diagnose if the number of arguments is too large for
     /// the current number of 'for' loops.
     bool TileDepthSatisfied = true;
@@ -177,6 +177,21 @@ class SemaOpenACC : public SemaBase {
 
   void CheckLastRoutineDeclNameConflict(const NamedDecl *ND);
 
+  bool DiagnoseRequiredClauses(OpenACCDirectiveKind DK, SourceLocation DirLoc,
+                               ArrayRef<const OpenACCClause *> Clauses);
+
+  bool DiagnoseAllowedClauses(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
+                              SourceLocation ClauseLoc);
+
+public:
+  // Needed from the visitor, so should be public.
+  bool DiagnoseAllowedOnceClauses(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
+                                  SourceLocation ClauseLoc,
+                                  ArrayRef<const OpenACCClause *> Clauses);
+  bool DiagnoseExclusiveClauses(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
+                                SourceLocation ClauseLoc,
+                                ArrayRef<const OpenACCClause *> Clauses);
+
 public:
   ComputeConstructInfo &getActiveComputeConstructInfo() {
     return ActiveComputeConstructInfo;

clang/lib/AST/OpenACCClause.cpp

@@ -190,18 +190,15 @@ OpenACCCollapseClause::OpenACCCollapseClause(SourceLocation BeginLoc,
                                              SourceLocation EndLoc)
     : OpenACCClauseWithSingleIntExpr(OpenACCClauseKind::Collapse, BeginLoc,
                                      LParenLoc, LoopCount, EndLoc),
-      HasForce(HasForce) {
-  assert(LoopCount && "LoopCount required");
-}
+      HasForce(HasForce) {}
 
 OpenACCCollapseClause *
 OpenACCCollapseClause::Create(const ASTContext &C, SourceLocation BeginLoc,
                               SourceLocation LParenLoc, bool HasForce,
                               Expr *LoopCount, SourceLocation EndLoc) {
-  assert(
-      LoopCount &&
-      (LoopCount->isInstantiationDependent() || isa<ConstantExpr>(LoopCount)) &&
-      "Loop count not constant expression");
+  assert((!LoopCount || (LoopCount->isInstantiationDependent() ||
+                         isa<ConstantExpr>(LoopCount))) &&
+         "Loop count not constant expression");
   void *Mem =
       C.Allocate(sizeof(OpenACCCollapseClause), alignof(OpenACCCollapseClause));
   return new (Mem)

clang/lib/AST/TextNodeDumper.cpp

@@ -433,6 +433,7 @@ void TextNodeDumper::Visit(const OpenACCClause *C) {
     case OpenACCClauseKind::Vector:
     case OpenACCClauseKind::VectorLength:
     case OpenACCClauseKind::Invalid:
+    case OpenACCClauseKind::Shortloop:
       // The condition expression will be printed as a part of the 'children',
       // but print 'clause' here so it is clear what is happening from the dump.
       OS << " clause";

clang/lib/Parse/ParseOpenACC.cpp

@@ -537,6 +537,8 @@ ClauseParensKind getClauseParensKind(OpenACCDirectiveKind DirKind,
   case OpenACCClauseKind::Tile:
     return ClauseParensKind::Required;
 
+  case OpenACCClauseKind::Shortloop:
+    llvm_unreachable("Shortloop shouldn't be generated in clang");
   case OpenACCClauseKind::Auto:
   case OpenACCClauseKind::Finalize:
   case OpenACCClauseKind::IfPresent:

clang/lib/Sema/CMakeLists.txt

@@ -75,6 +75,7 @@ add_clang_library(clangSema
   SemaOpenACC.cpp
   SemaOpenACCAtomic.cpp
   SemaOpenACCClause.cpp
+  SemaOpenACCClauseAppertainment.cpp
   SemaOpenCL.cpp
   SemaOpenMP.cpp
   SemaOverload.cpp