[SYCL] Implement SYCL_ONEAPI_accessor_properties #2456

alexbatashev · 2020-09-10T11:15:37Z

Implement SYCL runtime part of SYCL_ONEAPI_accessor_properties extension (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/accessor_properties/SYCL_ONEAPI_accessor_properties.asciidoc). Also add buffer_location property (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/accessor_properties/SYCL_INTEL_buffer_location.asciidoc).

alexbatashev · 2020-09-10T11:16:55Z

This PR requires #2447 to be merged. Right now it contains some unrelated changes to run tests. Runtime changes begin at 71462a44815ce6884cf1794c522f1220eecd30d5

sycl/include/CL/sycl/image.hpp

Signed-off-by: Alexander Batashev <[email protected]>

vladimirlaz · 2020-09-14T10:42:36Z

please provide link to extension documentation in PR.

alexbatashev · 2020-09-14T10:46:52Z

please provide link to extension documentation in PR.

Done.

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp

Co-authored-by: Mikhail Lychkov <[email protected]>

sycl/test/basic_tests/accessor/accessor_property_list.cpp

vladimirlaz · 2020-09-15T06:20:07Z

sycl/test/abi/layout_accessors.cpp

@@ -1,6 +1,8 @@
 // RUN: %clangxx -fsycl -c -fno-color-diagnostics -Xclang -fdump-record-layouts %s | FileCheck %s
 // REQUIRES: linux


is there any reason to not run it on windows?

Windows version only differs in class names (due to different standard library implementation)

* origin/sycl: [SYCL] Fixes in release notes (intel#2470) [SYCL] Refactor -fsycl-esimd and -fsycl-int-header options (intel#2466)

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp

sycl/include/CL/sycl/detail/property_list_base.hpp

turinevgeny

types.hpp looks good.

Co-authored-by: Mikhail Lychkov <[email protected]>

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp

s-kanaev · 2020-09-15T14:12:36Z

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp

+            AreSameTemplate<PropT, Head>::value ||
+                !sycl::detail::IsCompileTimePropertyInstance<PropT>::value,


Now, if the PropT and Head are not the same and PropT isn't a compile-time property this condition will evaluate to true, isn't it? Is it even correct?

It was done to skip runtime properties, but that was probably the wrong place. Moved.

s-kanaev · 2020-09-15T14:19:02Z

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp

+            !std::is_same_v<typename ContainerT::Head, void> &&
+                (AreSameTemplate<PropT<Args...>,
+                                 typename ContainerT::Head>::value ||
+                 !sycl::detail::IsCompileTimePropertyInstance<
+                     typename ContainerT::Head>::value),


Same question here

sycl/include/CL/sycl/ONEAPI/reduction.hpp

s-kanaev · 2020-09-15T14:44:29Z

sycl/include/CL/sycl/accessor.hpp

+template <typename T>
+using IsRunTimePropertyListT =
+    typename std::is_same<ONEAPI::accessor_property_list<>, T>;


Are there no runtime properties available?

Before this patch, accessor constructors could accept property_list. To allow transparent conversions for user from property_list to some accessor_property_list, I had to make some differentiation between accessor_property_list with template parameters (aka constexpr parameters) and without (aka run time parameters). Runtime parameters do not require any template arguments, because they're stored in run time container.

sycl/include/CL/sycl/properties/accessor_properties.hpp

MrSidims

The patch is missing at least 1 at max 2 test(s).
First test needs to go to fpga_tests. Scenario goes as follows:
create an accessor with buffer_location property, write to this accessor and then read on host, checking the result (the test might have some simple calculations). The purpose is to ensure, that the property doesn't break anything at runtime level (even if it's no-op for cpu/gpu/fpga emu).
Second test (not required, but at least it's needed to ensure, that is works correctly locally) is for checking device code for buffer_location feature aka checking if the metadata kernel_arg_buffer_location presents in the module (I know, that there is a test in clang, but it works with a dummy header, need to check with the real one).

alexbatashev · 2020-09-16T09:08:05Z

The patch is missing at least 1 at max 2 test(s).
First test needs to go to fpga_tests. Scenario goes as follows:
create an accessor with buffer_location property, write to this accessor and then read on host, checking the result (the test might have some simple calculations). The purpose is to ensure, that the property doesn't break anything at runtime level (even if it's no-op for cpu/gpu/fpga emu).
Second test (not required, but at least it's needed to ensure, that is works correctly locally) is for checking device code for buffer_location feature aka checking if the metadata kernel_arg_buffer_location presents in the module (I know, that there is a test in clang, but it works with a dummy header, need to check with the real one).

Added two more tests.

MrSidims

LGTM
just one minor comment and a minor question/testing scenario idea that can be addressed later.

sycl/test/fpga_tests/buffer_location_codegen.cpp

sycl/test/basic_tests/accessor/accessor_property_list_ct.cpp

* upstream/sycl: (405 commits) [SYCL] Implement new env var SYCL_DEVICE_FILTER (intel#2239) [Driver][SYCL] Make /MD the default for -fsycl (intel#2478) [SYCL]: basic support of contexts with multiple devices in Level-Zero (intel#2440) [SYCL] Fix LIT regression after 9dd18ca (intel#2481) [SYCL][L0] Kernel Destroy in piKernelRelease (intel#2475) [SYCL] Emit an aliased function only if it is used (intel#2430) [Driver][SYCL] Add defaultlib directive for sycl lib (intel#2464) [Driver][SYCL] Improve situations where .exe is added for AOT tools (intel#2467) [SYCL][L0]: Check Queue refcnt prior to using members in event wait/release (intel#2471) [SYCL] Unroll several loops in __init method accessor class (intel#2449) [SYCL][Doc] Add link to use pinned memory spec (intel#2463) [SYCL] Link SYCL device libraries by default. (intel#2400) Revert "[SYCL] XFAIL test blcoking pulldown" Avoid usage of deprecated "VectorType::getNumElements" (intel#737) Fix nullptr dereference (intel#741) Do not translate arbitrary precision operations without corresponding extensions (intel#714) Add Constrained Floating-Point Intrinsics support [SYCL] Take into account auxiliary cmake options for Level Zero loader [InstCombine] improve fold of pointer differences [InstCombine] add ptr difference tests; NFC ...

alexbatashev · 2020-09-17T07:45:29Z

@s-kanaev @turinevgeny ping

vladimirlaz · 2020-09-17T09:50:38Z

sycl/test/fpga_tests/buffer_location.cpp

@@ -0,0 +1,22 @@
+// RUN: %clangxx -fsycl %s -o %t.out
+// RUNx: %ACC_RUN_PLACEHOLDER %t.out


Is test execution disabled by any reason. Could you please add the comment with description if it is the case

No, it's a typo. If no objections, I'll do a followup patch to fix that.

OK for me to fix in separate patch

[benchmarks] add umf suite

alexbatashev requested a review from MrSidims September 10, 2020 11:17

alexbatashev commented Sep 11, 2020

View reviewed changes

sycl/include/CL/sycl/image.hpp Outdated Show resolved Hide resolved

Alexander Batashev added 13 commits September 14, 2020 11:46

[SYCL] Implement SYCL_ONEAPI_accessor_properties extension

d0d2a86

Signed-off-by: Alexander Batashev <[email protected]>

Minor fix

415c939

Update for latest revision

41365d7

Fix tests

ee55a6a

More clang-format

976dfec

Fix template arguments

33da077

Fix more missing templates

5dba583

clang-format

809061d

Propagate template arguments to accessor_common

e9b9246

More clang-format

ccb172f

Remove unused code

ff28dc2

Fix empty prop list lookup

49028d5

Fix include

c22997b

alexbatashev force-pushed the accessor_property_list branch from 34c06d4 to c22997b Compare September 14, 2020 08:47

alexbatashev marked this pull request as ready for review September 14, 2020 08:48

alexbatashev requested review from turinevgeny and a team as code owners September 14, 2020 08:48

alexbatashev requested a review from vladimirlaz September 14, 2020 08:48

mlychkov reviewed Sep 14, 2020

View reviewed changes

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp Outdated Show resolved Hide resolved

Fix small typo

c4cebba

Co-authored-by: Mikhail Lychkov <[email protected]>

vladimirlaz suggested changes Sep 15, 2020

View reviewed changes

vladimirlaz requested a review from s-kanaev September 15, 2020 06:41

Alexander Batashev added 3 commits September 15, 2020 10:43

Split CT and RT tests

0091796

Attempt to improve code readability

d70980c

Merge remote-tracking branch 'origin/sycl' into accessor_property_list

3277faa

* origin/sycl: [SYCL] Fixes in release notes (intel#2470) [SYCL] Refactor -fsycl-esimd and -fsycl-int-header options (intel#2466)

mlychkov reviewed Sep 15, 2020

View reviewed changes

sycl/include/CL/sycl/ONEAPI/accessor_property_list.hpp Outdated Show resolved Hide resolved

sycl/include/CL/sycl/detail/property_list_base.hpp Outdated Show resolved Hide resolved

turinevgeny previously approved these changes Sep 15, 2020

View reviewed changes

Apply suggestions from code review

2c04881

Co-authored-by: Mikhail Lychkov <[email protected]>

alexbatashev dismissed turinevgeny’s stale review via 2c04881 September 15, 2020 11:25

turinevgeny previously approved these changes Sep 15, 2020

View reviewed changes

s-kanaev reviewed Sep 15, 2020

View reviewed changes

MrSidims reviewed Sep 15, 2020

View reviewed changes

Address more comments

ed8efd7

alexbatashev dismissed turinevgeny’s stale review via ed8efd7 September 16, 2020 07:18

Fix merge artifacts

e982a29

MrSidims approved these changes Sep 16, 2020

View reviewed changes

sycl/test/fpga_tests/buffer_location_codegen.cpp Show resolved Hide resolved

sycl/test/basic_tests/accessor/accessor_property_list_ct.cpp Show resolved Hide resolved

s-kanaev approved these changes Sep 17, 2020

View reviewed changes

vladimirlaz approved these changes Sep 17, 2020

View reviewed changes

mlychkov approved these changes Sep 18, 2020

View reviewed changes

alexbatashev requested a review from turinevgeny September 18, 2020 06:51

turinevgeny approved these changes Sep 18, 2020

View reviewed changes

romanovvlad merged commit f7d073d into intel:sycl Sep 18, 2020

alexbatashev deleted the accessor_property_list branch October 31, 2020 09:43

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2456 from EuphoricThinking/benchmark_job6_final_pr

df0aa84

[benchmarks] add umf suite

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Implement SYCL_ONEAPI_accessor_properties #2456

[SYCL] Implement SYCL_ONEAPI_accessor_properties #2456

alexbatashev commented Sep 10, 2020 •

edited

Loading

alexbatashev commented Sep 10, 2020 •

edited

Loading

vladimirlaz commented Sep 14, 2020

alexbatashev commented Sep 14, 2020

vladimirlaz Sep 15, 2020

alexbatashev Sep 15, 2020

turinevgeny left a comment

s-kanaev Sep 15, 2020

alexbatashev Sep 16, 2020

s-kanaev Sep 15, 2020

s-kanaev Sep 15, 2020

MrSidims Sep 15, 2020

alexbatashev Sep 16, 2020

MrSidims left a comment

alexbatashev commented Sep 16, 2020

MrSidims left a comment

alexbatashev commented Sep 17, 2020

vladimirlaz Sep 17, 2020

alexbatashev Sep 17, 2020

vladimirlaz Sep 17, 2020

		@@ -1,6 +1,8 @@
		// RUN: %clangxx -fsycl -c -fno-color-diagnostics -Xclang -fdump-record-layouts %s \| FileCheck %s
		// REQUIRES: linux

		AreSameTemplate<PropT, Head>::value \|\|
		!sycl::detail::IsCompileTimePropertyInstance<PropT>::value,

		@@ -0,0 +1,22 @@
		// RUN: %clangxx -fsycl %s -o %t.out
		// RUNx: %ACC_RUN_PLACEHOLDER %t.out

[SYCL] Implement SYCL_ONEAPI_accessor_properties #2456

[SYCL] Implement SYCL_ONEAPI_accessor_properties #2456

Conversation

alexbatashev commented Sep 10, 2020 • edited Loading

alexbatashev commented Sep 10, 2020 • edited Loading

vladimirlaz commented Sep 14, 2020

alexbatashev commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turinevgeny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims left a comment

Choose a reason for hiding this comment

alexbatashev commented Sep 16, 2020

MrSidims left a comment

Choose a reason for hiding this comment

alexbatashev commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbatashev commented Sep 10, 2020 •

edited

Loading

alexbatashev commented Sep 10, 2020 •

edited

Loading