[SYCL] Specific error messages for invalid properties on non pointer …

…types (#11843) This change adds specific error messages for invalid properties that are specified on non pointer types. This change is directly in the header annotated_arg.hpp and these checks are done before the `check_property_list` check which previously accounted for this but had a generic error message. This change adds static asserts so that the error message specifies the invalid property and that it is invalid due to a non pointer type for the underlying data type. For example, if the user tries to declare the following variable: ```cpp annotated_arg<int, decltype(properties{buffer_location<0>})> a; ``` The following error message is outputted: ``` error: static assertion failed due to requirement '!has_buffer_location': Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type. 288 | static_assert(!has_buffer_location, | ^~~~~~~~~~~~~~~~~~~~ ``` Prior to this change, the following generic error message was printed with multiple notes: ``` /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:59:17: error: static assertion failed due to requirement 'is_valid_property_for_given_type': Property is invalid for the given type. 59 | static_assert(is_valid_property_for_given_type, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:307:7: note: in instantiation of template class 'sycl::ext::oneapi::experimental::check_property_list<int, sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>' requested here 307 | check_property_list<T, Props...>::value; | ^ /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:308:17: note: in instantiation of static data member 'sycl::ext::oneapi::experimental::annotated_arg<int, sycl::ext::oneapi::experimental::properties<std::tuple<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>>>::contains_valid_properties' requested here 308 | static_assert(contains_valid_properties, | ^ temp.cpp:15:66: note: in instantiation of template class 'sycl::ext::oneapi::experimental::annotated_arg<int, sycl::ext::oneapi::experimental::properties<std::tuple<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>>>' requested here 15 | annotated_arg<int, decltype(properties{buffer_location<0>})> a; | ^ ``` ### Alternate Methods PR intel/llvm#11748 adds the same checks but in the `fpga_annotated_properties.hpp` file as a separate check. This PR has the checks directly in the `annotated_arg.hpp` header file so that there are fewer notes printed below the error message. Here is a comparison of how the error message is outputted when the checks are in different files: With error checks in properties header file and using a macro (PR intel/llvm#11748): ``` /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp:396:3: error: static assertion failed due to requirement '!has_buffer_location': Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type. 396 | CHECK_INVALID_PROPERTY(buffer_location, list) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp:389:17: note: expanded from macro 'CHECK_INVALID_PROPERTY' 389 | static_assert(!has_##property, \ | ^~~~~~~~~~~~~~~ /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:202:25: note: in instantiation of template class 'sycl::ext::oneapi::experimental::detail::checkPropertiesForNonPointerType<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>' requested here 202 | static_assert(detail::checkPropertiesForNonPointerType<Props...>::value); | ^ temp.cpp:15:66: note: in instantiation of template class 'sycl::ext::oneapi::experimental::annotated_arg<int, sycl::ext::oneapi::experimental::properties<std::tuple<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>>>' requested here 15 | annotated_arg<int, decltype(properties{buffer_location<0>})> a; | ^ ``` With error check in annotated_arg header and using a macro: ``` /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:292:3: error: static assertion failed due to requirement '!has_buffer_location': Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type. 292 | CHECK_INVALID_PROPERTY(buffer_location, property_tuple) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:23:17: note: expanded from macro 'CHECK_INVALID_PROPERTY' 23 | static_assert(!has_##property, \ | ^~~~~~~~~~~~~~~ temp.cpp:15:66: note: in instantiation of template class 'sycl::ext::oneapi::experimental::annotated_arg<int, sycl::ext::oneapi::experimental::properties<std::tuple<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>>>' requested here 15 | annotated_arg<int, decltype(properties{buffer_location<0>})> a; | ^ ``` ### This Change With error check in annotated_arg header and not using a macro (This PR): ``` /p/psg/swip/w/yugteshs/sycl_include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:288:17: error: static assertion failed due to requirement '!has_buffer_location': Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type. 288 | static_assert(!has_buffer_location, | ^~~~~~~~~~~~~~~~~~~~ temp.cpp:15:66: note: in instantiation of template class 'sycl::ext::oneapi::experimental::annotated_arg<int, sycl::ext::oneapi::experimental::properties<std::tuple<sycl::ext::oneapi::experimental::property_value<sycl::ext::intel::experimental::buffer_location_key, std::integral_constant<int, 0>>>>>' requested here 15 | annotated_arg<int, decltype(properties{buffer_location<0>})> a; | ^ ``` This version only prints one note and therefore it is more clear for the user.
wenhuiNi · Nov 14, 2023 · 728b132 · 728b132
1 parent c0291b2
commit 728b132
Show file tree

Hide file tree

Showing 5 changed files with 170 additions and 74 deletions.
diff --git a/sycl/include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp b/sycl/include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp
@@ -10,6 +10,7 @@
 
 #include <sycl/detail/defines.hpp>
 #include <sycl/ext/intel/experimental/fpga_annotated_properties.hpp>
+#include <sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr_properties.hpp>
 #include <sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp>
 #include <sycl/ext/oneapi/properties/properties.hpp>
 
@@ -84,26 +85,6 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T *, detail::properties_t<Props...>> {
 #endif
 
 public:
-  static constexpr bool is_valid_property_list =
-      is_property_list<property_list_t>::value;
-  static_assert(is_valid_property_list, "Property list is invalid.");
-  static constexpr bool contains_valid_properties =
-      check_property_list<T *, Props...>::value;
-  static_assert(contains_valid_properties,
-                "The property list contains invalid property.");
-  // check the set if FPGA specificed properties are used
-  static constexpr bool hasValidFPGAProperties =
-      detail::checkValidFPGAPropertySet<Props...>::value;
-  static_assert(hasValidFPGAProperties,
-                "FPGA Interface properties (i.e. awidth, dwidth, etc.)"
-                "can only be set with BufferLocation together.");
-  // check if conduit and register_map properties are specified together
-  static constexpr bool hasConduitAndRegisterMapProperties =
-      detail::checkHasConduitAndRegisterMap<Props...>::value;
-  static_assert(hasConduitAndRegisterMapProperties,
-                "The properties conduit and register_map cannot be "
-                "specified at the same time.");
-
   annotated_arg() noexcept = default;
   annotated_arg(const annotated_arg &) = default;
   annotated_arg &operator=(annotated_arg &) = default;
@@ -191,6 +172,34 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T *, detail::properties_t<Props...>> {
   template <typename PropertyT> static constexpr auto get_property() {
     return property_list_t::template get_property<PropertyT>();
   }
+
+  // *************************************************************************
+  // All static error checking is added here instead of placing inside neat
+  // functions to minimize the number lines printed out when an assert
+  // is triggered.
+  // static constexprs are used to ensure that the triggered assert prints
+  // a message that is very readable. Without these, the assert will
+  // print out long templated names
+  // *************************************************************************
+  static constexpr bool is_valid_property_list =
+      is_property_list<property_list_t>::value;
+  static_assert(is_valid_property_list, "Property list is invalid.");
+  static constexpr bool contains_valid_properties =
+      check_property_list<T *, Props...>::value;
+  static_assert(contains_valid_properties,
+                "The property list contains invalid property.");
+  // check the set if FPGA specificed properties are used
+  static constexpr bool hasValidFPGAProperties =
+      detail::checkValidFPGAPropertySet<Props...>::value;
+  static_assert(hasValidFPGAProperties,
+                "FPGA Interface properties (i.e. awidth, dwidth, etc.) "
+                "can only be set with BufferLocation together.");
+  // check if conduit and register_map properties are specified together
+  static constexpr bool hasConduitAndRegisterMapProperties =
+      detail::checkHasConduitAndRegisterMap<Props...>::value;
+  static_assert(hasConduitAndRegisterMapProperties,
+                "The properties conduit and register_map cannot be "
+                "specified at the same time.");
 };
 
 // Partial specialization for non-pointer type
@@ -212,28 +221,6 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T, detail::properties_t<Props...>> {
 #endif
 
 public:
-  static constexpr bool is_device_copyable = is_device_copyable_v<T>;
-  static_assert(is_device_copyable, "Type T must be device copyable.");
-  static constexpr bool is_valid_property_list =
-      is_property_list<property_list_t>::value;
-  static_assert(is_valid_property_list, "Property list is invalid.");
-  static constexpr bool contains_valid_properties =
-      check_property_list<T, Props...>::value;
-  static_assert(contains_valid_properties,
-                "The property list contains invalid property.");
-  // check the set if FPGA specificed properties are used
-  static constexpr bool hasValidFPGAProperties =
-      detail::checkValidFPGAPropertySet<Props...>::value;
-  static_assert(hasValidFPGAProperties,
-                "FPGA Interface properties (i.e. awidth, dwidth, etc.)"
-                "can only be set with BufferLocation together.");
-  // check if conduit and register_map properties are specified together
-  static constexpr bool hasConduitAndRegisterMapProperties =
-      detail::checkHasConduitAndRegisterMap<Props...>::value;
-  static_assert(hasConduitAndRegisterMapProperties,
-                "The properties conduit and register_map cannot be "
-                "specified at the same time.");
-
   annotated_arg() noexcept = default;
   annotated_arg(const annotated_arg &) = default;
   annotated_arg &operator=(annotated_arg &) = default;
@@ -383,6 +370,82 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T, detail::properties_t<Props...>> {
   R operator<<(const annotated_arg<T2, PropertyList2> &other) const {
     return obj << other.obj;
   }
+
+  // *************************************************************************
+  // All static error checking is added here instead of placing inside neat
+  // functions to minimize the number lines printed out when an assert
+  // is triggered.
+  // static constexprs are used to ensure that the triggered assert prints
+  // a message that is very readable. Without these, the assert will
+  // print out long templated names
+  // *************************************************************************
+  static constexpr bool is_device_copyable = is_device_copyable_v<T>;
+  static_assert(is_device_copyable, "Type T must be device copyable.");
+
+  // check if invalid properties are specified for non pointer type
+  static constexpr bool has_buffer_location =
+      has_property<buffer_location_key>();
+  static_assert(!has_buffer_location,
+                "Property buffer_location cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_awidth = has_property<awidth_key>();
+  static_assert(!has_awidth, "Property awidth cannot be specified for "
+                             "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_dwidth = has_property<dwidth_key>();
+  static_assert(!has_dwidth, "Property dwidth cannot be specified for "
+                             "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_latency = has_property<latency_key>();
+  static_assert(!has_latency, "Property latency cannot be specified for "
+                              "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_read_write_mode =
+      has_property<read_write_mode_key>();
+  static_assert(!has_read_write_mode,
+                "Property read_write_mode cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_maxburst = has_property<maxburst_key>();
+  static_assert(!has_maxburst,
+                "Property maxburst cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_wait_request = has_property<wait_request_key>();
+  static_assert(!has_wait_request,
+                "Property wait_request cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_alignment = has_property<alignment_key>();
+  static_assert(!has_alignment,
+                "Property alignment cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool has_usm_kind = has_property<usm_kind_key>();
+  static_assert(!has_usm_kind,
+                "Property usm_kind cannot be specified for "
+                "annotated_arg<T> when T is a non pointer type.");
+
+  static constexpr bool is_valid_property_list =
+      is_property_list<property_list_t>::value;
+  static_assert(is_valid_property_list, "Property list is invalid.");
+  static constexpr bool contains_valid_properties =
+      check_property_list<T, Props...>::value;
+  static_assert(contains_valid_properties,
+                "The property list contains invalid property.");
+  // check the set if FPGA specificed properties are used
+  static constexpr bool hasValidFPGAProperties =
+      detail::checkValidFPGAPropertySet<Props...>::value;
+  static_assert(hasValidFPGAProperties,
+                "FPGA Interface properties (i.e. awidth, dwidth, etc.) "
+                "can only be set with BufferLocation together.");
+  // check if conduit and register_map properties are specified together
+  static constexpr bool hasConduitAndRegisterMapProperties =
+      detail::checkHasConduitAndRegisterMap<Props...>::value;
+  static_assert(hasConduitAndRegisterMapProperties,
+                "The properties conduit and register_map cannot be "
+                "specified at the same time.");
 };
 
 template <typename T, typename PropertyList, typename T2,

diff --git a/sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr.hpp b/sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr.hpp
@@ -219,26 +219,6 @@ __SYCL_TYPE(annotated_ptr) annotated_ptr<T, detail::properties_t<Props...>> {
 #endif
 
 public:
-  static constexpr bool is_valid_property_list =
-      is_property_list<property_list_t>::value;
-  static_assert(is_valid_property_list, "Property list is invalid.");
-  static constexpr bool contains_valid_properties =
-      check_property_list<T *, Props...>::value;
-  static_assert(contains_valid_properties,
-                "The property list contains invalid property.");
-  // check the set if FPGA specificed properties are used
-  static constexpr bool hasValidFPGAProperties =
-      detail::checkValidFPGAPropertySet<Props...>::value;
-  static_assert(hasValidFPGAProperties,
-                "FPGA Interface properties (i.e. awidth, dwidth, etc.)"
-                "can only be set with BufferLocation together.");
-  // check if conduit and register_map properties are specified together
-  static constexpr bool hasConduitAndRegisterMapProperties =
-      detail::checkHasConduitAndRegisterMap<Props...>::value;
-  static_assert(hasConduitAndRegisterMapProperties,
-                "The properties conduit and register_map cannot be "
-                "specified at the same time.");
-
   annotated_ptr() noexcept = default;
   annotated_ptr(const annotated_ptr &) = default;
   annotated_ptr &operator=(const annotated_ptr &) = default;
@@ -360,6 +340,34 @@ __SYCL_TYPE(annotated_ptr) annotated_ptr<T, detail::properties_t<Props...>> {
   template <typename PropertyT> static constexpr auto get_property() {
     return property_list_t::template get_property<PropertyT>();
   }
+
+  // *************************************************************************
+  // All static error checking is added here instead of placing inside neat
+  // functions to minimize the number lines printed out when an assert
+  // is triggered.
+  // static constexprs are used to ensure that the triggered assert prints
+  // a message that is very readable. Without these, the assert will
+  // print out long templated names
+  // *************************************************************************
+  static constexpr bool is_valid_property_list =
+      is_property_list<property_list_t>::value;
+  static_assert(is_valid_property_list, "Property list is invalid.");
+  static constexpr bool contains_valid_properties =
+      check_property_list<T *, Props...>::value;
+  static_assert(contains_valid_properties,
+                "The property list contains invalid property.");
+  // check the set if FPGA specificed properties are used
+  static constexpr bool hasValidFPGAProperties =
+      detail::checkValidFPGAPropertySet<Props...>::value;
+  static_assert(hasValidFPGAProperties,
+                "FPGA Interface properties (i.e. awidth, dwidth, etc.) "
+                "can only be set with BufferLocation together.");
+  // check if conduit and register_map properties are specified together
+  static constexpr bool hasConduitAndRegisterMapProperties =
+      detail::checkHasConduitAndRegisterMap<Props...>::value;
+  static_assert(hasConduitAndRegisterMapProperties,
+                "The properties conduit and register_map cannot be "
+                "specified at the same time.");
 };
 
 } // namespace experimental

diff --git a/sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr_properties.hpp b/sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr_properties.hpp
@@ -9,6 +9,7 @@
 
 #pragma once
 
+#include <sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp>
 #include <sycl/ext/oneapi/properties/properties.hpp> // for properties_t
 #include <sycl/usm/usm_enums.hpp>
 

diff --git a/sycl/test/extensions/annotated_arg/annotated_arg_negative.cpp b/sycl/test/extensions/annotated_arg/annotated_arg_negative.cpp
@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s
+// RUN: %clangxx -fsycl -ferror-limit=0 -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s
 
 #include "sycl/sycl.hpp"
 #include <sycl/ext/intel/fpga_extensions.hpp>
@@ -17,7 +17,42 @@ void check_conduit_and_register_map_properties() {
   annotated_ptr<int, decltype(properties{conduit, register_map})> c;
 }
 
+void check_invalid_properties_on_non_pointer_types() {
+  // check buffer location property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{buffer_location<0>})> a;
+
+  // check awidth property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property awidth cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{awidth<32>})> b;
+
+  // check dwidth property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property dwidth cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{dwidth<32>})> c;
+
+  // check latency property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property latency cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{latency<1>})> d;
+
+  // check read_write_mode property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property read_write_mode cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{read_write_mode_readwrite})> e;
+
+  // check maxburst property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property maxburst cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{maxburst<1>})> f;
+
+  // check wait_request property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property wait_request cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{wait_request_requested})> g;
+
+  // check alignment property specified on non pointer type
+  // expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{Property alignment cannot be specified for annotated_arg<T> when T is a non pointer type.}}
+  annotated_arg<int, decltype(properties{alignment<256>})> h;
+}
+
 int main() {
+  check_invalid_properties_on_non_pointer_types();
   check_conduit_and_register_map_properties();
   return 0;
 }
diff --git a/sycl/test/extensions/annotated_arg/annotated_arg_properties.cpp b/sycl/test/extensions/annotated_arg/annotated_arg_properties.cpp
@@ -46,15 +46,6 @@ template <typename T> void checkIsPropertyOf() {
       is_property_value_of<decltype(wait_request_requested), T>::value);
 }
 
-// Checks is_property_key_of and is_property_value_of are false for non-pointer
-// type T.
-template <typename T> void checkIsValidPropertyOfNonPtr() {
-  static_assert(
-      is_valid_property<T, decltype(wait_request_not_requested)>::value ==
-      false);
-  static_assert(is_valid_property<T, decltype(latency<8>)>::value == false);
-}
-
 int main() {
   static_assert(is_property_key<register_map_key>::value);
   static_assert(is_property_key<buffer_location_key>::value);
@@ -90,7 +81,5 @@ int main() {
   static_assert(AnnotatedArg4.get_property<read_write_mode_key>() ==
                 read_write_mode_read);
 
-  // Check if a property is valid for a given type
-  checkIsValidPropertyOfNonPtr<A>();
   return 0;
 }