Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 17 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5445994
adding static assertions for properties on non pointer type and remov…
yug-intel Nov 1, 2023
508efe7
added static assertion for alignment
yug-intel Nov 1, 2023
d5606eb
adding negative test for checking invalid props on non pointer types
yug-intel Nov 1, 2023
b9be0e4
fix lint
yug-intel Nov 1, 2023
d5f2340
fix bug in annotated_arg_properties.cpp
yug-intel Nov 2, 2023
2894df0
Using macro to save lines of code
yug-intel Nov 3, 2023
d323c2c
fix lint
yug-intel Nov 6, 2023
2fec364
added back is_valid_property and check_property_list check
yug-intel Nov 6, 2023
91aa229
added include
yug-intel Nov 6, 2023
6c21846
fix lint
yug-intel Nov 6, 2023
fe098fe
restored all definitions for is_valid_property
yug-intel Nov 7, 2023
06be97a
added include again
yug-intel Nov 7, 2023
f60ca37
Merge branch 'sycl' into specific-error-message-for-invalid-property
yug-intel Nov 7, 2023
d32a982
testing removing include
yug-intel Nov 8, 2023
7e216c5
updated annotated_arg_negative.cpp test
yug-intel Nov 8, 2023
6917f19
added no error limit for annotated_arg_negative.cpp
yug-intel Nov 8, 2023
d3a0381
testing includes
yug-intel Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions sycl/include/sycl/ext/intel/experimental/fpga_annotated_properties.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#pragma once

#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>
#include <sycl/ext/oneapi/properties/property.hpp>
Expand Down Expand Up @@ -382,6 +383,27 @@ template <typename... Args> struct checkHasConduitAndRegisterMap {
ContainsProperty<register_map_key, list>::value;
static constexpr bool value = !(has_Conduit && has_RegisterMap);
};

#define CHECK_INVALID_PROPERTY(property, list) \
static constexpr bool has_##property = \
ContainsProperty<property##_key, list>::value; \
static_assert(!has_##property, \
"Property " #property " cannot be specified for " \
"annotated_arg<T> when T is a non pointer type.");

template <typename... Args>
Copy link
Contributor

@broxigarchen broxigarchen Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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
  2. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@tiwaria1 tiwaria1 Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

struct checkPropertiesForNonPointerType : std::true_type {
using list = std::tuple<Args...>;
CHECK_INVALID_PROPERTY(buffer_location, list)
CHECK_INVALID_PROPERTY(awidth, list)
CHECK_INVALID_PROPERTY(dwidth, list)
CHECK_INVALID_PROPERTY(latency, list)
CHECK_INVALID_PROPERTY(read_write_mode, list)
CHECK_INVALID_PROPERTY(maxburst, list)
CHECK_INVALID_PROPERTY(wait_request, list)
CHECK_INVALID_PROPERTY(alignment, list)
CHECK_INVALID_PROPERTY(usm_kind, list)
};
} // namespace detail

} // namespace experimental
Expand Down
6 changes: 4 additions & 2 deletions sycl/include/sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T *, detail::properties_t<Props...>> {
static constexpr bool hasValidFPGAProperties =
detail::checkValidFPGAPropertySet<Props...>::value;
static_assert(hasValidFPGAProperties,
"FPGA Interface properties (i.e. awidth, dwidth, etc.)"
"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 =
Expand Down Expand Up @@ -214,6 +214,8 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T, detail::properties_t<Props...>> {
public:
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_assert(detail::checkPropertiesForNonPointerType<Props...>::value);
static constexpr bool is_valid_property_list =
is_property_list<property_list_t>::value;
static_assert(is_valid_property_list, "Property list is invalid.");
Expand All @@ -225,7 +227,7 @@ __SYCL_TYPE(annotated_arg) annotated_arg<T, detail::properties_t<Props...>> {
static constexpr bool hasValidFPGAProperties =
detail::checkValidFPGAPropertySet<Props...>::value;
static_assert(hasValidFPGAProperties,
"FPGA Interface properties (i.e. awidth, dwidth, etc.)"
"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 =
Expand Down
1 change: 1 addition & 0 deletions sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr_properties.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
51 changes: 50 additions & 1 deletion sycl/test/extensions/annotated_arg/annotated_arg_negative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
Expand All @@ -17,7 +17,56 @@ 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/intel/experimental/fpga_annotated_properties.hpp:* {{Property buffer_location cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
annotated_arg<int, decltype(properties{buffer_location<0>})> a;

// check awidth property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property awidth cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{awidth<32>})> b;

// check dwidth property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property dwidth cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{dwidth<32>})> c;

// check latency property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property latency cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{latency<1>})> d;

// check read_write_mode property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property read_write_mode cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{read_write_mode_readwrite})> e;

// check maxburst property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property maxburst cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{maxburst<1>})> f;

// check wait_request property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property wait_request cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given type.}}
// expected-error@sycl/ext/oneapi/experimental/annotated_arg/annotated_arg.hpp:* {{FPGA Interface properties (i.e. awidth, dwidth, etc.) can only be set with BufferLocation together.}}
annotated_arg<int, decltype(properties{wait_request_requested})> g;

// check alignment property specified on non pointer type
// expected-error@sycl/ext/intel/experimental/fpga_annotated_properties.hpp:* {{Property alignment cannot be specified for annotated_arg<T> when T is a non pointer type.}}
// expected-error@sycl/ext/oneapi/experimental/common_annotated_properties/properties.hpp:* {{Property is invalid for the given 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;
}
11 changes: 0 additions & 11 deletions sycl/test/extensions/annotated_arg/annotated_arg_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}