[SYCL] Fix mechanism to throw exception when placeholder accessor passed to a command (#10110)

Current mechanism for checking whether an exception should be thrown if
a placeholder accessor was passed to a command was not working properly
under certain conditions, either by not throwing when required, or
throwing when unrequired. Additionally, following clarifications in
KhronosGroup/SYCL-Docs#434, this PR makes
default-constructed accessors to stop being placeholders.

---------

Signed-off-by: Maronas, Marcos <[email protected]>

Author: maarquitos14

sycl/source/handler.cpp

@@ -122,22 +122,23 @@ event handler::finalize() {
 
   // According to 4.7.6.9 of SYCL2020 spec, if a placeholder accessor is passed
   // to a command without being bound to a command group, an exception should
-  // be thrown. There should be as many requirements as unique accessors,
-  // otherwise some of the accessors are unbound, and thus we throw.
+  // be thrown.
   {
-    // A counter is not good enough since we can have the same accessor several
-    // times as arg
-    std::unordered_set<void *> accessors;
     for (const auto &arg : MArgs) {
       if (arg.MType != detail::kernel_param_kind_t::kind_accessor)
         continue;
 
-      accessors.insert(arg.MPtr);
+      detail::Requirement *AccImpl =
+          static_cast<detail::Requirement *>(arg.MPtr);
+      if (AccImpl->MIsPlaceH) {
+        auto It = std::find(CGData.MRequirements.begin(),
+                            CGData.MRequirements.end(), AccImpl);
+        if (It == CGData.MRequirements.end())
+          throw sycl::exception(make_error_code(errc::kernel_argument),
+                                "placeholder accessor must be bound by calling "
+                                "handler::require() before it can be used.");
+      }
     }
-    if (accessors.size() > CGData.MRequirements.size())
-      throw sycl::exception(make_error_code(errc::kernel_argument),
-                            "placeholder accessor must be bound by calling "
-                            "handler::require() before it can be used.");
   }
 
   const auto &type = getType();

sycl/test-e2e/Basic/accessor/accessor.cpp

@@ -696,7 +696,7 @@ int main() {
           acc(b);
 
       q.submit([&](sycl::handler &cgh) {
-        // we do NOT call .require(acc) without which we should throw a
+        // We do NOT call .require(acc) without which we should throw a
         // synchronous exception with errc::kernel_argument
         cgh.parallel_for<class ph1>(r,
                                     [=](sycl::id<1> index) { acc[index] = 0; });
@@ -727,7 +727,7 @@ int main() {
       AccT acc(b);
 
       q.submit([&](sycl::handler &cgh) {
-        // we do NOT call .require(acc) without which we should throw a
+        // We do NOT call .require(acc) without which we should throw a
         // synchronous exception with errc::kernel_argument
         // The difference with the previous test is that the use of acc
         // is usually optimized away for this particular scenario, but the
@@ -762,14 +762,14 @@ int main() {
 
       q.submit([&](sycl::handler &cgh) {
         AccT acc2(b, cgh);
-        // we do NOT call .require(acc) without which we should throw a
+        // We do NOT call .require(acc) without which we should throw a
         // synchronous exception with errc::kernel_argument
         // The particularity of this test is that it passes to a command
         // one bound accessor and one unbound accessor. In the past, this
         // has led to throw the wrong exception.
         cgh.single_task<class ph3>([=] {
-          volatile int x = acc[0];
-          volatile int y = acc2[0];
+          int x = acc[0];
+          int y = acc2[0];
         });
       });
       q.wait_and_throw();
@@ -784,6 +784,40 @@ int main() {
     }
   }
 
+  // placeholder accessor exception (4)  // SYCL2020 4.7.6.9
+  {
+    sycl::queue q;
+    // host device executes kernels via a different method and there
+    // is no good way to throw an exception at this time.
+    sycl::range<1> r(4);
+    sycl::buffer<int, 1> b(r);
+    try {
+      using AccT = sycl::accessor<int, 1, sycl::access::mode::read_write,
+                                  sycl::access::target::device,
+                                  sycl::access::placeholder::true_t>;
+      AccT acc(b);
+
+      q.submit([&](sycl::handler &cgh) {
+        AccT acc2(b, cgh);
+        // Pass placeholder accessor to command, but having required a different
+        // accessor in the command. In past versions, we used to compare the
+        // number of accessors with the number of requirements, and if they
+        // matched, we did not throw, allowing this scenario that shouldn't be
+        // allowed.
+        cgh.single_task<class ph4>([=] { int x = acc[0]; });
+      });
+      q.wait_and_throw();
+      assert(false && "we should not be here, missing exception");
+    } catch (sycl::exception &e) {
+      std::cout << "exception received: " << e.what() << std::endl;
+      assert(e.code() == sycl::errc::kernel_argument && "incorrect error code");
+    } catch (...) {
+      std::cout << "Some other exception (line " << __LINE__ << ")"
+                << std::endl;
+      return 1;
+    }
+  }
+
   // SYCL2020 4.9.4.1: calling require() on empty accessor should throw
   {
     sycl::queue q;
@@ -1430,5 +1464,34 @@ int main() {
     }
   }
 
+  // default constructed accessor is not a placeholder
+  {
+    AccT acc;
+    assert(!acc.is_placeholder());
+    sycl::queue q;
+    bool result;
+    {
+      sycl::buffer<bool, 1> Buf{&result, sycl::range<1>{1}};
+      // As a non-placeholder accessor, make sure no exception is thrown when
+      // passed to a command. However, we cannot access it, because there's
+      // no underlying storage.
+      try {
+        q.submit([&](sycl::handler &cgh) {
+          sycl::accessor res_acc{Buf, cgh};
+          cgh.single_task<class def_ctor>(
+              [=] { res_acc[0] = acc.is_placeholder(); });
+        });
+        q.wait_and_throw();
+      } catch (sycl::exception &e) {
+        assert("Unexpected exception");
+      } catch (...) {
+        std::cout << "Some other unexpected exception (line " << __LINE__ << ")"
+                  << std::endl;
+        return 1;
+      }
+    }
+    assert(!result);
+  }
+
   std::cout << "Test passed" << std::endl;
 }