The device expression must evaluate to a non-negative integer value.

[Harshil-Jani]

Summary

Device clause when it occurs with target enter data and target exit data must be declared with some non negative value. So some changes were made to evaluate the device clause argument to non negative value and throw the expected error when it takes negative value as argument.

[clementval]

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

39

Please use RequiresPositiveParameter. It does the same check.

[clementval]

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

39

If you want to accept 0 as well it probably better to add an argument to the current check

[clementval]

flang/lib/Semantics/check-directive-structure.h

553

The diff looks odd. Looks like you just uploaded the change over your previous patch. You should squash your commits so we can see the change compare to the current main.

[clementval]

The precheck is still failing. Please have a look why the patch application fails.

flang/lib/Semantics/check-directive-structure.h

557

Since you want to accept 0 please add a valid test with 0 so that we will catch any change in the future.

llvm/lib/Support/FormatVariadic.cpp

21 ↗

(On Diff #406775)

Unrelated change. Please remove.

[clementval]

Can you add a valid test for 0?

[clementval]

LGTM. Just move the valid test into the correct file.

flang/test/Semantics/omp-device-constructs.f90

144

Please move the valid test into this file instead. https://github.com/llvm/llvm-project/blob/main/flang/test/Semantics/omp-clause-validity01.f90

[clementval]

Still LGTM.

flang/lib/Semantics/check-directive-structure.h

protected:
   std::string ContextDirectiveAsFortran();
 
   void RequiresConstantPositiveParameter(
       const C &clause, const parser::ScalarIntConstantExpr &i);
 
   void RequiresPositiveParameter(const C &clause,
       const parser::ScalarIntExpr &i, llvm::StringRef paramName = "parameter");
 
+  void RequiresNonNegativeParameter(const C &clause,
+      const parser::ScalarIntExpr &i, llvm::StringRef paramName = "parameter");
+
   void OptionalConstantPositiveParameter(
       const C &clause, const std::optional<parser::ScalarIntConstantExpr> &o);
 
   virtual llvm::StringRef getClauseName(C clause) { return ""; };
 
   virtual llvm::StringRef getDirectiveName(D directive) { return ""; };
 
   SemanticsContext &context_;
context_
       .Attach(beginSource, "Does not match directive"_en_US);
 }
 
 // Check the value of the clause is a positive parameter.
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 void DirectiveStructureChecker<D, C, PC,
     ClauseEnumSize>::RequiresPositiveParameter(const C &clause,
     const parser::ScalarIntExpr &i, llvm::StringRef paramName) {
   if (const auto v{GetIntValue(i)}) {
     if (*v <= 0) {
       context_.Say(GetContext().clauseSource,
           "The %s of the %s clause must be "
           "a positive integer expression"_err_en_US,
           paramName.str(),
           parser::ToUpperCaseLetters(getClauseName(clause).str()));
     }
   }
 }
 
+// Check the value of the clause is a non negative parameter.
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+void DirectiveStructureChecker<D, C, PC,
+    ClauseEnumSize>::RequiresNonNegativeParameter(const C &clause,
+    const parser::ScalarIntExpr &i, llvm::StringRef paramName) {
+  if (const auto v{GetIntValue(i)}) {
+    if (*v < 0) {
+      context_.Say(GetContext().clauseSource,
+          "The %s of the %s clause must be "
+          "a non-negative integer expression"_err_en_US,
+          paramName.str(),
+          parser::ToUpperCaseLetters(getClauseName(clause).str()));
+    }
+  }
+}
+
 } // namespace Fortran::semantics
 
 #endif // FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_

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

#define CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(X, Y) \
   }
 
 #define CHECK_REQ_SCALAR_INT_CLAUSE(X, Y) \
   void OmpStructureChecker::Enter(const parser::OmpClause::X &c) { \
     CheckAllowed(llvm::omp::Clause::Y); \
     RequiresPositiveParameter(llvm::omp::Clause::Y, c.v); \
   }
 
+#define CHECK_REQ_NON_NEGATIVE_SCALAR_INT_CLAUSE(X, Y) \
+  \ 
+  void \
+  OmpStructureChecker::Enter(const parser::OmpClause::X &c) { \
+    CheckAllowed(llvm::omp::Clause::Y); \
+    RequiresNonNegativeParameter(llvm::omp::Clause::Y, c.v); \
+  }
+
 // Use when clause don't falls under 'struct OmpClause' in 'parse-tree.h'.
 #define CHECK_SIMPLE_PARSER_CLAUSE(X, Y) \
   void OmpStructureChecker::Enter(const parser::X &) { \
     CheckAllowed(llvm::omp::Y); \
   }
 
 // 'OmpWorkshareBlockChecker' is used to check the validity of the assignment
 // statements and the expressions enclosed in an OpenMP Workshare construct

 CHECK_SIMPLE_CLAUSE(Affinity, OMPC_affinity)
 CHECK_SIMPLE_CLAUSE(Allocate, OMPC_allocate)
 CHECK_SIMPLE_CLAUSE(Capture, OMPC_capture)
 CHECK_SIMPLE_CLAUSE(Copyin, OMPC_copyin)
 CHECK_SIMPLE_CLAUSE(Default, OMPC_default)
 CHECK_SIMPLE_CLAUSE(Depobj, OMPC_depobj)
 CHECK_SIMPLE_CLAUSE(Destroy, OMPC_destroy)
 CHECK_SIMPLE_CLAUSE(Detach, OMPC_detach)
-CHECK_SIMPLE_CLAUSE(Device, OMPC_device)
 CHECK_SIMPLE_CLAUSE(DeviceType, OMPC_device_type)
 CHECK_SIMPLE_CLAUSE(DistSchedule, OMPC_dist_schedule)
 CHECK_SIMPLE_CLAUSE(DynamicAllocators, OMPC_dynamic_allocators)
 CHECK_SIMPLE_CLAUSE(Exclusive, OMPC_exclusive)
 CHECK_SIMPLE_CLAUSE(Final, OMPC_final)
 CHECK_SIMPLE_CLAUSE(Flush, OMPC_flush)
 CHECK_SIMPLE_CLAUSE(From, OMPC_from)
 CHECK_SIMPLE_CLAUSE(Full, OMPC_full)

 CHECK_SIMPLE_CLAUSE(Compare, OMPC_compare)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
 CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
 CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
-
 CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Collapse, OMPC_collapse)
 CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Safelen, OMPC_safelen)
 CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Simdlen, OMPC_simdlen)
 
+CHECK_REQ_NON_NEGATIVE_SCALAR_INT_CLAUSE(Device, OMPC_device)
+
 // Restrictions specific to each clause are implemented apart from the
 // generalized restrictions.
 void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_reduction);
   if (CheckReductionOperators(x)) {
     CheckReductionTypeList(x);
   }
 }

flang/test/Semantics/omp-device-constructs.f90

program main
 
   !ERROR: At most one DEVICE clause can appear on the TARGET directive
   !$omp target device(0) device(1)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end target
 
+  !ERROR: The parameter of the DEVICE clause must be a non-negative integer expression
+  !$omp target enter data map(alloc:A) device(-2)
+
+  !ERROR: The parameter of the DEVICE clause must be a non-negative integer expression
+  !$omp target exit data map(delete:A) device(-2)
+
   !ERROR: SCHEDULE clause is not allowed on the TARGET directive
   !$omp target schedule(static)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end target
 
   !$omp target defaultmap(tofrom:scalar)
program main
   !$omp end target data
 
   !ERROR: At most one IF clause can appear on the TARGET ENTER DATA directive
   !$omp target enter data map(to:a) if(.true.) if(.false.)
 
   !ERROR: Only the TO, ALLOC map types are permitted for MAP clauses on the TARGET ENTER DATA directive
   !$omp target enter data map(from:a)
 
   !$omp target exit data map(delete:a)
 
   !ERROR: At most one DEVICE clause can appear on the TARGET EXIT DATA directive
   !$omp target exit data map(from:a) device(0) device(1)
 
   !ERROR: Only the FROM, RELEASE, DELETE map types are permitted for MAP clauses on the TARGET EXIT DATA directive
   !$omp target exit data map(to:a)
 
   !$omp target