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

554

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

138

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

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) {
+    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()));
     }
   }
 }
 
 } // namespace Fortran::semantics
 
 #endif // FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_

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

void OmpStructureChecker::Enter(const parser::OmpClause::X &c) { \
     RequiresPositiveParameter(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
 class OmpWorkshareBlockChecker {
 public:
   OmpWorkshareBlockChecker(SemanticsContext &context, parser::CharBlock source)
       : context_{context}, source_{source} {}
 
   template <typename T> bool Pre(const T &) { return true; }

 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_SCALAR_INT_CLAUSE(Device, OMPC_device)
 
 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)
 
 // Restrictions specific to each clause are implemented apart from the
 // generalized restrictions.
 void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {

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

!                               simd-clause
   enddo
 
   !ERROR: The parameter of the SIMDLEN clause must be a constant positive integer expression
   !ERROR: The parameter of the ALIGNED clause must be a constant positive integer expression
   !$omp taskloop simd simdlen(-1) aligned(a:-2)
   do i = 1, N
      a = 3.14
   enddo
+
+  !$omp target enter data map(alloc:A) device(0) 
+  !$omp target exit data map(delete:A) device(0) 
+
 end program

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

program main
 
   !ERROR: At least one of MAP clause must appear on the TARGET DATA directive
   !$omp target data device(0)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end target data
 
+  !ERROR: The parameter of the DEVICE clause must be a positive integer expression
+  !$omp target enter data map(alloc:A) device(-2)
+
+  !ERROR: The parameter of the DEVICE clause must be a positive integer expression
+  !$omp target exit data map(delete:A) device(-2)
+
   !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