[flang][openacc] Add clause validity tests for the update directive

[clementval]

Summary

Add couple of clause validity tests for the update directive and check for
the restriction where at least self, host or device clause must appear on the directive.

[sameeranjoshi]

Looks Good Thanks.

[clementval]

Rebase

[clementval]

Revert to previous version after a bad rebase/update

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

#include "llvm/Frontend/OpenACC/ACC.cpp.inc"
   NODE(parser, AccObject)
   NODE(parser, AccObjectList)
   NODE(parser, AccObjectListWithModifier)
   NODE(parser, AccObjectListWithReduction)
   NODE(parser, AccReductionOperator)
   NODE_ENUM(parser::AccReductionOperator, Operator)
   NODE(parser, AccSizeExpr)
   NODE(parser, AccSizeExprList)
+  NODE(parser, AccSelfClause)
   NODE(parser, AccStandaloneDirective)
   NODE(parser, AccTileExpr)
   NODE(parser, AccTileExprList)
   NODE(parser, AccLoopDirective)
   NODE(parser, AccWaitArgument)
   static std::string GetNodeName(const llvm::acc::Directive &x) {
     return llvm::Twine(
         "llvm::acc::Directive = ", llvm::acc::getOpenACCDirectiveName(x))

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

struct AccSizeExpr {
   CharBlock source;
   std::tuple<std::optional<ScalarIntExpr>> t; // if null then *
 };
 
 struct AccSizeExprList {
   WRAPPER_CLASS_BOILERPLATE(AccSizeExprList, std::list<AccSizeExpr>);
 };
 
+struct AccSelfClause {
+  UNION_CLASS_BOILERPLATE(AccSelfClause);
+  std::variant<std::optional<ScalarLogicalExpr>, AccObjectList> u;
+  CharBlock source;
+};
+
 struct AccGangArgument {
   TUPLE_CLASS_BOILERPLATE(AccGangArgument);
   std::tuple<std::optional<ScalarIntExpr>, std::optional<AccSizeExpr>> t;
 };
 
 struct AccClause {
   UNION_CLASS_BOILERPLATE(AccClause);
 

flang/lib/Lower/OpenACC.cpp

for (const auto &clause : accClauseList.v) {
     } else if (const auto *ifClause =
                    std::get_if<Fortran::parser::AccClause::If>(&clause.u)) {
       Value cond = fir::getBase(
           converter.genExprValue(*Fortran::semantics::GetExpr(ifClause->v)));
       ifCond = firOpBuilder.createConvert(currentLocation,
                                           firOpBuilder.getI1Type(), cond);
     } else if (const auto *selfClause =
                    std::get_if<Fortran::parser::AccClause::Self>(&clause.u)) {
-      if (selfClause->v) {
+      const Fortran::parser::AccSelfClause &accSelfClause = selfClause->v;
+      if (const auto *optCondition =
+              std::get_if<std::optional<Fortran::parser::ScalarLogicalExpr>>(
+                  &accSelfClause.u)) {
+        if (*optCondition) {
           Value cond = fir::getBase(converter.genExprValue(
-            *Fortran::semantics::GetExpr(*(selfClause->v))));
+              *Fortran::semantics::GetExpr(*optCondition)));
           selfCond = firOpBuilder.createConvert(currentLocation,
                                                 firOpBuilder.getI1Type(), cond);
         } else {
           addSelfAttr = true;
         }
+      }
     } else if (const auto *copyClause =
                    std::get_if<Fortran::parser::AccClause::Copy>(&clause.u)) {
       genObjectList(copyClause->v, converter, copyOperands);
     } else if (const auto *copyinClause =
                    std::get_if<Fortran::parser::AccClause::Copyin>(&clause.u)) {
       genObjectListWithModifier<Fortran::parser::AccClause::Copyin>(
           copyinClause, converter,
           Fortran::parser::AccDataModifier::Modifier::ReadOnly,

flang/lib/Parser/openacc-parsers.cpp

"PRESENT" >> construct<AccClause>(construct<AccClause::Present>(
                      parenthesized(Parser<AccObjectList>{}))) ||
     "PRIVATE" >> construct<AccClause>(construct<AccClause::Private>(
                      parenthesized(Parser<AccObjectList>{}))) ||
     "READ" >> construct<AccClause>(construct<AccClause::Read>()) ||
     "REDUCTION" >> construct<AccClause>(construct<AccClause::Reduction>(
                        parenthesized(construct<AccObjectListWithReduction>(
                            Parser<AccReductionOperator>{} / ":",
                            Parser<AccObjectList>{})))) ||
-    // SELF clause is either a simple optional condition for compute construct
-    // or a synonym of the HOST clause for the update directive 2.14.4 holding
-    // an object list.
-    "SELF" >> construct<AccClause>(construct<AccClause::Self>(
-                  maybe(parenthesized(scalarLogicalExpr)))) ||
-    construct<AccClause>(
-        construct<AccClause::Host>(parenthesized(Parser<AccObjectList>{}))) ||
+    "SELF" >> construct<AccClause>(
+                  construct<AccClause::Self>(Parser<AccSelfClause>{})) ||
     "SEQ" >> construct<AccClause>(construct<AccClause::Seq>()) ||
     "TILE" >> construct<AccClause>(construct<AccClause::Tile>(
                   parenthesized(Parser<AccTileExprList>{}))) ||
     "USE_DEVICE" >> construct<AccClause>(construct<AccClause::UseDevice>(
                         parenthesized(Parser<AccObjectList>{}))) ||
     "VECTOR_LENGTH" >> construct<AccClause>(construct<AccClause::VectorLength>(
                            parenthesized(scalarIntExpr))) ||
     "VECTOR" >>
first("+" >> pure(AccReductionOperator::Operator::Plus),
         ".EQV." >> pure(AccReductionOperator::Operator::Eqv),
         ".NEQV." >> pure(AccReductionOperator::Operator::Neqv)))))
 
 // 2.5.14 Default clause
 TYPE_PARSER(construct<AccDefaultClause>(
     parenthesized(first("NONE" >> pure(AccDefaultClause::Arg::None),
         "PRESENT" >> pure(AccDefaultClause::Arg::Present)))))
 
+// SELF clause is either a simple optional condition for compute construct
+// or a synonym of the HOST clause for the update directive 2.14.4 holding
+// an object list.
+TYPE_PARSER(construct<AccSelfClause>(parenthesized(Parser<AccObjectList>{})) ||
+    construct<AccSelfClause>(maybe(parenthesized(scalarLogicalExpr))))
+
 // Modifier for copyin, copyout, cache and create
 TYPE_PARSER(construct<AccDataModifier>(
     first("ZERO:" >> pure(AccDataModifier::Modifier::Zero),
         "READONLY:" >> pure(AccDataModifier::Modifier::ReadOnly))))
 
 // Combined directives
 TYPE_PARSER(sourced(construct<AccCombinedDirective>(
     first("KERNELS LOOP" >> pure(llvm::acc::Directive::ACCD_kernels_loop),

flang/lib/Semantics/check-acc-structure.cpp

void AccStructureChecker::Leave(const parser::OpenACCStandaloneConstruct &x) {
   case llvm::acc::Directive::ACCD_exit_data:
   case llvm::acc::Directive::ACCD_set:
     // Restriction - 1117-1118 (ENTER DATA)
     // Restriction - 1161-1162 (EXIT DATA)
     // Restriction - 2254 (SET)
     CheckRequireAtLeastOneOf();
     break;
   case llvm::acc::Directive::ACCD_update:
+    // Restriction - line 2636
+    CheckRequireAtLeastOneOf();
     // Restriction - 2301
     CheckOnlyAllowedAfter(llvm::acc::Clause::ACCC_device_type,
         updateOnlyAllowedAfterDeviceTypeClauses);
     break;
   default:
     break;
   }
   dirContext_.pop_back();

 CHECK_SIMPLE_CLAUSE(NoCreate, ACCC_no_create)
 CHECK_SIMPLE_CLAUSE(Nohost, ACCC_nohost)
 CHECK_SIMPLE_CLAUSE(NumGangs, ACCC_num_gangs)
 CHECK_SIMPLE_CLAUSE(NumWorkers, ACCC_num_workers)
 CHECK_SIMPLE_CLAUSE(Present, ACCC_present)
 CHECK_SIMPLE_CLAUSE(Private, ACCC_private)
 CHECK_SIMPLE_CLAUSE(Read, ACCC_read)
 CHECK_SIMPLE_CLAUSE(Reduction, ACCC_reduction)
-CHECK_SIMPLE_CLAUSE(Self, ACCC_self)
 CHECK_SIMPLE_CLAUSE(Seq, ACCC_seq)
 CHECK_SIMPLE_CLAUSE(Tile, ACCC_tile)
 CHECK_SIMPLE_CLAUSE(UseDevice, ACCC_use_device)
 CHECK_SIMPLE_CLAUSE(Vector, ACCC_vector)
 CHECK_SIMPLE_CLAUSE(VectorLength, ACCC_vector_length)
 CHECK_SIMPLE_CLAUSE(Wait, ACCC_wait)
 CHECK_SIMPLE_CLAUSE(Worker, ACCC_worker)
 CHECK_SIMPLE_CLAUSE(Write, ACCC_write)
if (modifier->v != parser::AccDataModifier::Modifier::Zero) {
           parser::ToUpperCaseLetters(
               llvm::acc::getOpenACCClauseName(llvm::acc::Clause::ACCC_copyout)
                   .str()),
           ContextDirectiveAsFortran());
     }
   }
 }
 
+void AccStructureChecker::Enter(const parser::AccClause::Self &x) {
+  CheckAllowed(llvm::acc::Clause::ACCC_self);
+  const parser::AccSelfClause &accSelfClause = x.v;
+  if (GetContext().directive == llvm::acc::Directive::ACCD_update &&
+      std::holds_alternative<std::optional<parser::ScalarLogicalExpr>>(
+          accSelfClause.u)) {
+    context_.Say(GetContext().clauseSource,
+        "SELF clause on the %s directive must have a var-list"_err_en_US,
+        ContextDirectiveAsFortran());
+  } else if (GetContext().directive != llvm::acc::Directive::ACCD_update &&
+      std::holds_alternative<parser::AccObjectList>(accSelfClause.u)) {
+    const auto &accObjectList =
+        std::get<parser::AccObjectList>(accSelfClause.u);
+    if (accObjectList.v.size() != 1) {
+      context_.Say(GetContext().clauseSource,
+          "SELF clause on the %s directive only accepts optional scalar logical"
+          " expression"_err_en_US,
+          ContextDirectiveAsFortran());
+    }
+  }
+}
+
 llvm::StringRef AccStructureChecker::getClauseName(llvm::acc::Clause clause) {
   return llvm::acc::getOpenACCClauseName(clause);
 }
 
 llvm::StringRef AccStructureChecker::getDirectiveName(
     llvm::acc::Directive directive) {
   return llvm::acc::getOpenACCDirectiveName(directive);
 }
 
 } // namespace Fortran::semantics

flang/test/Semantics/acc-clause-validity.f90

program openacc_clause_validity
   !ERROR: At most one IF clause can appear on the DATA directive
   !$acc data copy(aa) if(.true.) if(ifCondition)
   !$acc end data
 
   !$acc data copyin(i)
   !ERROR: Unmatched PARALLEL directive
   !$acc end parallel
 
+  !ERROR: At least one of DEVICE, HOST, SELF clause must appear on the UPDATE directive
+  !$acc update
+
   !$acc update self(a, f) host(g) device(h)
 
-  !$acc update device(i) device_type(*) async
+  !$acc update host(aa) async(1)
+
+  !$acc update device(bb) async(async1)
+
+  !ERROR: At most one ASYNC clause can appear on the UPDATE directive
+  !$acc update host(aa, bb) async(1) async(2)
+
+  !$acc update self(bb, cc(:)) wait(1)
+
+  !ERROR: SELF clause on the UPDATE directive must have a var-list
+  !$acc update self
+
+  !$acc update device(aa, bb, cc) wait(wait1)
+
+  !$acc update host(aa) host(bb) device(cc) wait(1,2)
+
+  !$acc update device(aa, cc) wait(wait1, wait2)
+
+  !$acc update device(aa) device_type(*) async
+
+  !$acc update host(bb) device_type(*) wait
+
+  !$acc update self(cc) device_type(1,2) async device_type(3) wait
+
+  !ERROR: At most one IF clause can appear on the UPDATE directive
+  !$acc update device(aa) if(.true.) if(ifCondition)
+
+  !ERROR: At most one IF_PRESENT clause can appear on the UPDATE directive
+  !$acc update device(bb) if_present if_present
 
   !ERROR: Clause IF is not allowed after clause DEVICE_TYPE on the UPDATE directive
   !$acc update device(i) device_type(*) if(.TRUE.)
 
   !$acc parallel
   !ERROR: INDEPENDENT and SEQ clauses are mutually exclusive and may not appear on the same LOOP directive
   !$acc loop seq independent
   do i = 1, N
do i = 1, N
     a(i) = 3.14
   end do
 
   !$acc parallel loop self
   do i = 1, N
     a(i) = 3.14
   end do
 
+  !ERROR: SELF clause on the PARALLEL LOOP directive only accepts optional scalar logical expression
+  !$acc parallel loop self(bb, cc(:))
+  do i = 1, N
+    a(i) = 3.14
+  end do
+
   !$acc parallel loop self(.true.)
   do i = 1, N
     a(i) = 3.14
   end do
 
   !$acc parallel loop self(ifCondition)
   do i = 1, N
     a(i) = 3.14

llvm/include/llvm/Frontend/OpenACC/ACC.td

 
 // 2.5.13
 def ACCC_Reduction : Clause<"reduction"> {
   let flangClassValue = "AccObjectListWithReduction";
 }
 
 // 2.5.5
 def ACCC_Self : Clause<"self"> {
-  let flangClassValue = "ScalarLogicalExpr";
-  let isValueOptional = true;
+  let flangClassValue = "AccSelfClause";
 }
 
 // 2.9.5
 def ACCC_Seq : Clause<"seq"> {}
 
 // 2.9.4
 def ACCC_Vector : Clause<"vector"> {
   let flangClassValue = "ScalarIntExpr";