[SYCL] Adding specific error messages for invalid property on non pointer types #11748

yug-intel · 2023-11-01T19:38:46Z

When there is an invalid property specified on a non pointer type (such as buffer_location, awidth, dwidth,...), there is a very generic error message outputted: "Property is invalid for the given type." 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. The original error message is still printed in addition.

For example, if the user tries to declare the following variable:

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.
  387 |   CHECK_INVALID_PROPERTY(buffer_location, list)
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ajaykumarkannan

Can you add negative tests for these error messages?

dm-vodopyanov · 2023-11-01T20:02:43Z

@yug-intel please follow https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#commit-message

broxigarchen · 2023-11-02T18:18:37Z

sycl/include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp

@@ -373,6 +335,45 @@ template <typename... Args> struct checkValidFPGAPropertySet {

  static constexpr bool value = !(!has_BufferLocation && has_InterfaceConfig);
 };
+template <typename... Args>


I think this might not be the way we want to do this check, mainly for two reasons:

We want to avoid this large trunk of code. It will be hard to maintain if the list get longer and longer. Instead we should use the template to auto-generate this check function in pre-processor

This requires the whole property list to be checked all the time. When the list is long, this will be quite inefficient. With the template way, the code only checks properties that are given by the user code

I checked with Yug and it seems we didn't find a way to get the template to work with static_assert.

The problem is that the pre-processor runs before template processing so you can not get a string customized with template's type and display it on a static_assert.

Let's just create a simple Macro here and makes it a bit cleaner and it should be good to go

I saw the error that comes with the macro style. I disagree, we should keep Yug's original version. It is strictly better from a user's perspective.

I have pushed the macro change. I talked with Abhishek offline and he is okay with it now because the error message does not contain "_key" and is now very similar to what was being printed before.

broxigarchen

I think we need to fix the check_property function and improve the error message instead of moving them all into a big check function

tiwaria1 · 2023-11-02T18:54:38Z

I think we need to fix the check_property function and improve the error message instead of moving them all into a big check function

@broxigarchen If you have a better method in mind please let Yug know. My main concern is user experience and I do not mind a long function if it helps us achieve that efficiently.

sycl/include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp

…ing is_valid_property check

dm-vodopyanov · 2023-11-05T17:10:51Z

@yug-intel please, don't do force-push in this project, it's hard for reviewers to see what was changed in the PR after their comments.

yug-intel · 2023-11-06T19:58:43Z

@dm-vodopyanov Sorry, I did a rebase so I had force-pushed

yug-intel · 2023-11-10T18:01:38Z

Moving forward with PR #11843

…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 #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 #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.

yug-intel requested a review from a team as a code owner November 1, 2023 19:38

yug-intel requested review from dm-vodopyanov and tiwaria1 November 1, 2023 19:38

ajaykumarkannan reviewed Nov 1, 2023

View reviewed changes

yug-intel changed the title ~~Adding specific error messages for invalid property on non pointer types~~ [SYCL] Adding specific error messages for invalid property on non pointer types Nov 1, 2023

yug-intel had a problem deploying to WindowsCILock November 1, 2023 21:33 — with GitHub Actions Failure

yug-intel temporarily deployed to WindowsCILock November 1, 2023 22:54 — with GitHub Actions Inactive

yug-intel had a problem deploying to WindowsCILock November 2, 2023 18:17 — with GitHub Actions Failure

broxigarchen reviewed Nov 2, 2023

View reviewed changes

broxigarchen suggested changes Nov 2, 2023

View reviewed changes

tiwaria1 reviewed Nov 2, 2023

View reviewed changes

sycl/include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp Outdated Show resolved Hide resolved

yug-intel added 6 commits November 3, 2023 13:58

adding static assertions for properties on non pointer type and remov…

5445994

…ing is_valid_property check

added static assertion for alignment

508efe7

adding negative test for checking invalid props on non pointer types

d5606eb

fix lint

b9be0e4

fix bug in annotated_arg_properties.cpp

d5f2340

Using macro to save lines of code

2894df0

yug-intel force-pushed the specific-error-message-for-invalid-property branch from 7c408a3 to 2894df0 Compare November 3, 2023 21:41

fix lint

d323c2c

yug-intel had a problem deploying to WindowsCILock November 6, 2023 15:15 — with GitHub Actions Failure

added back is_valid_property and check_property_list check

2fec364

yug-intel requested a review from wangdi4 November 6, 2023 19:39

yug-intel had a problem deploying to WindowsCILock November 6, 2023 20:03 — with GitHub Actions Failure

yug-intel added 2 commits November 6, 2023 13:04

added include

91aa229

fix lint

6c21846

yug-intel had a problem deploying to WindowsCILock November 6, 2023 21:46 — with GitHub Actions Failure

yug-intel had a problem deploying to WindowsCILock November 6, 2023 22:54 — with GitHub Actions Failure

yug-intel added 2 commits November 7, 2023 08:14

restored all definitions for is_valid_property

fe098fe

added include again

06be97a

yug-intel had a problem deploying to WindowsCILock November 7, 2023 16:43 — with GitHub Actions Failure

Merge branch 'sycl' into specific-error-message-for-invalid-property

f60ca37

yug-intel had a problem deploying to WindowsCILock November 7, 2023 18:35 — with GitHub Actions Failure

testing removing include

d32a982

yug-intel had a problem deploying to WindowsCILock November 8, 2023 18:01 — with GitHub Actions Failure

yug-intel temporarily deployed to WindowsCILock November 8, 2023 18:51 — with GitHub Actions Inactive

updated annotated_arg_negative.cpp test

7e216c5

yug-intel had a problem deploying to WindowsCILock November 8, 2023 19:00 — with GitHub Actions Failure

added no error limit for annotated_arg_negative.cpp

6917f19

yug-intel had a problem deploying to WindowsCILock November 8, 2023 19:28 — with GitHub Actions Failure

testing includes

d3a0381

yug-intel temporarily deployed to WindowsCILock November 8, 2023 19:50 — with GitHub Actions Inactive

yug-intel requested review from ajaykumarkannan, broxigarchen and tiwaria1 November 8, 2023 20:34

yug-intel temporarily deployed to WindowsCILock November 8, 2023 21:06 — with GitHub Actions Inactive

yug-intel mentioned this pull request Nov 9, 2023

[SYCL] Specific error messages for invalid properties on non pointer types #11843

Merged

yug-intel closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Adding specific error messages for invalid property on non pointer types #11748

[SYCL] Adding specific error messages for invalid property on non pointer types #11748

Uh oh!

yug-intel commented Nov 1, 2023 •

edited

Loading

Uh oh!

ajaykumarkannan left a comment

Uh oh!

dm-vodopyanov commented Nov 1, 2023

Uh oh!

broxigarchen Nov 2, 2023 •

edited

Loading

Uh oh!

broxigarchen Nov 3, 2023

Uh oh!

tiwaria1 Nov 3, 2023 •

edited

Loading

Uh oh!

yug-intel Nov 3, 2023

Uh oh!

broxigarchen left a comment

Uh oh!

tiwaria1 commented Nov 2, 2023

Uh oh!

Uh oh!

dm-vodopyanov commented Nov 5, 2023

Uh oh!

yug-intel commented Nov 6, 2023

Uh oh!

yug-intel commented Nov 10, 2023

Uh oh!

Uh oh!

[SYCL] Adding specific error messages for invalid property on non pointer types #11748

[SYCL] Adding specific error messages for invalid property on non pointer types #11748

Uh oh!

Conversation

yug-intel commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajaykumarkannan left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov commented Nov 1, 2023

Uh oh!

broxigarchen Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broxigarchen Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

tiwaria1 Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yug-intel Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

broxigarchen left a comment

Choose a reason for hiding this comment

Uh oh!

tiwaria1 commented Nov 2, 2023

Uh oh!

Uh oh!

dm-vodopyanov commented Nov 5, 2023

Uh oh!

yug-intel commented Nov 6, 2023

Uh oh!

yug-intel commented Nov 10, 2023

Uh oh!

Uh oh!

yug-intel commented Nov 1, 2023 •

edited

Loading

broxigarchen Nov 2, 2023 •

edited

Loading

tiwaria1 Nov 3, 2023 •

edited

Loading