[SYCL] Implementation of discard_events #5026

alexanderfle · 2021-11-24T16:21:55Z

Review documentation in #4922

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle · 2021-11-26T13:45:48Z

/summary:run

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle · 2021-12-03T17:48:38Z

/summary:run

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle · 2021-12-07T10:42:46Z

/summary:run

vladimirlaz

The tests need to be added (both in sycl/test and intel/llvm-test-suite) for the new functionality.

sycl/include/CL/sycl/detail/memory_manager.hpp

sycl/include/CL/sycl/queue.hpp

romanovvlad · 2021-12-09T09:16:55Z

sycl/source/detail/event_impl.cpp

@@ -303,6 +312,9 @@ template <> cl_uint event_impl::get_info<info::event::reference_count>() const {
 template <>
 info::event_command_status
 event_impl::get_info<info::event::command_execution_status>() const {
+  if (MState == HES_Invalid)


Maybe it's better to say discarded rather than invalid? Invalid sounds like something wrong with the implementation.

Ok, I will change HES_Invalid to HES_Discarded

romanovvlad · 2021-12-09T09:20:02Z

sycl/source/detail/queue_impl.hpp

+  // The presence of support for DiscardEvents from the queue
+  const bool MHasDiscardEventsSupport;


Could you please clarify what this support means?

MHasDiscardEventsSupport serves to answer the question: can we discard the low-level event using this queue? The variable answers this question in terms of queue support, because the final decision about discarding is decided before the operation itself, since it is necessary to check whether this operation uses accessors, and also in the case of a kernel, it is necessary to check the use of assert() statement.

For example, you can create a queue with discard_events property, but without in_order property then the queue doesn't support discarding.
Another example, you can create a queue with discard_events and in_order properties, but run it on level_zero backend(which doesn't support discarding events yet) then the queue doesn't support discarding.

Ok, so this flags says if we can discard events based on a queue "setup" which will be common for all kernels submitted to the queue. And as you said this is a must condition for discarding, but even if it's true, in some cases, we won't be able to discard events.

It would be nice if this information is added as a comment.

romanovvlad · 2021-12-09T09:23:50Z

sycl/include/CL/sycl/handler.hpp

+    if (info::event_command_status::ext_oneapi_unknown ==
+        Event.get_info<info::event::command_execution_status>()) {


This looks very expensive. Since what we really would like to know here is whether the event is discarded, can we just check this field?

This code covers the case "when invalid event is passed into handler API" described in https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/DiscardQueueEvents/SYCL_EXT_ONEAPI_DISCARD_QUEUE_EVENTS.asciidoc

Here we just want to check if the MState field of event_impl has HES_Invalid value or not.
I think it is possible if we move the definition of the depends_on() method to the handler.cpp and check the field by the new public method of event_impl(it needs to be added). I will try that, thanks!

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

sycl/source/handler.cpp

…discarded" event Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

romanovvlad · 2021-12-10T09:27:23Z

sycl/source/detail/event_impl.cpp

+  if (MState == HES_Discarded)
+    throw sycl::exception(
+        make_error_code(errc::invalid),
+        "waitInternal method cannot be used for an discarded event.");


Suggested change

"waitInternal method cannot be used for an discarded event.");

"waitInternal method cannot be used for a discarded event.");

Done, thanks!

romanovvlad · 2021-12-10T09:48:52Z

sycl/include/CL/sycl/info/info_desc.hpp

+  ext_oneapi_unknown =
+      submitted + running + complete +
+      1 // TODO: a more elegant way to ensure that the unique value is here


Suggested change

ext_oneapi_unknown =

submitted + running + complete +

1 // TODO: a more elegant way to ensure that the unique value is here

ext_oneapi_unknown = 1000;

It would leave enough space for 1:1 mapping if BE supports more statuses.
Maybe @gmlueck have an opinion as well.

You are worried that the value of ext_oneapi_unknown might collide with some future OpenCL extension that adds a new "command execution status" value? It's hard to predict what new values might be added in the future. However, since all values are positive now, it's probably safe to use a negative value. Therefore, you could use -1. If we add other ext_oneapi values in the future, they could be -2, etc.

Ok, I changed it to -1, thanks for the suggestion!

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

romanovvlad

LGTM overall, but it would be nice if @vladimirlaz or @sergey-semenov take a look as well.

* upstream/sycl: (5961 commits) [SYCL] Implement discard_events extension (intel#5026) [SYCL][NFC] Fix unused parameter warning in piQueueFlush (intel#5139) [SYCL][XPTI] Fix static analysis tool warnings (intel#5040) [CI] Switch post-commit jobs to self-hosted runners (intel#5147) [SYCL] Fix support for classes implicitly converted from items in parallel_for (intel#5118) [SYCL][HIP] Fix platform query in USM alloc info (intel#5140) [Docker] Add workarounds for two SYCL issues (intel#5143) [CI] Install cm-compiler in drivers image (intel#5128) [ESIMD] Add support for an arbitrary number of elements to simd::copy_from/to (intel#5135) [SYCL] Add number HW threads per EU query (intel#4901) [CI] Refactor workflow files (intel#5134) [CI] Enable HIP and CUDA plugins in GitHub Actions builds (intel#5087) [SYCL] Implement queue flushing (intel#5052) Disable issue labeler in LLVM forks Modify translation for disable_loop_pipelining metadata Add SPIR-V friendly translation for OpLoad and OpStore Fix return type postfix for SPIR-V Friendly IR Restrict special handling of sampler OpVariable only to UniformConstant Add lowering for llvm.bswap intrinsic Fix translation of OpVariable with OpSamplerType ...

alexanderfle added 21 commits October 28, 2021 20:30

[SYCL] Add avoid_event_creation property for queue

adf4079

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Add exceptions for a few cases.

6905227

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Add check for kernel case

aaa71c8

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Merge branch 'sycl' into way1

04d798f

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Merge branch 'sycl' into way1

1b340b0

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Merge branch 'sycl' into way1

aa620d4

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Add new macro in Feature Test

f5730c6

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Update queue_properties.hpp according to last merge logic

2b1a221

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Rename avoid_event_creation property to discard_events

2f31823

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Change namespace on ext::oneapi::property::queue::discard_events

b08c309

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Add support for fallback assert

ebaa2a1

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Align implementation with spec

d325859

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Merge branch 'sycl' into way1

7f5de5b

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Clean up unnecessary changes

107721c

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Clean up unnecessary changes2

16cf4c0

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Fix ABI issues

3fd916e

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Update sycl_symbols for ABI test

675a1a3

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Update sycl_symbols_windows.dump

b0b1e0d

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Reduce one call to move assignment operator of sycl::event

23271fd

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Add mock for EventGetInfo

1a67f7d

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Fix not uniqueness of event_command_status enum

c996706

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

[SYCL] Update CommonRedefinitions in unittests

bf96adc

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle mentioned this pull request Dec 3, 2021

[SYCL] Add tests for discard_events feature intel/llvm-test-suite#599

Merged

[SYCL] Revert changes for MemoryManager::copy and MemoryManager::fill

fa654d3

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle added 3 commits December 6, 2021 10:34

Merge branch 'sycl' into discard_events_implementation

a85eec5

[SYCL] Update symbols for linux and windows according to previous revert

6e4468b

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

Merge branch 'sycl' into discard_events_implementation

65ef8fe

alexanderfle marked this pull request as ready for review December 9, 2021 07:31

alexanderfle requested a review from a team as a code owner December 9, 2021 07:31

alexanderfle requested a review from vladimirlaz December 9, 2021 07:31

vladimirlaz suggested changes Dec 9, 2021

View reviewed changes

sycl/include/CL/sycl/detail/memory_manager.hpp Show resolved Hide resolved

sycl/include/CL/sycl/queue.hpp Show resolved Hide resolved

romanovvlad reviewed Dec 9, 2021

View reviewed changes

[SYCL] Fix review comments

4cd8233

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

vladimirlaz suggested changes Dec 10, 2021

View reviewed changes

sycl/source/handler.cpp Outdated Show resolved Hide resolved

[SYCL] Update sycl_symbols_linux.dump and Rename "invalid" event to "…

9ec63ad

…discarded" event Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

vladimirlaz previously approved these changes Dec 10, 2021

View reviewed changes

[SYCL] Update comment to clarify "MHasDiscardEventsSupport"

d55f3e5

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle dismissed vladimirlaz’s stale review via d55f3e5 December 10, 2021 10:09

romanovvlad reviewed Dec 10, 2021

View reviewed changes

[SYCL] Update value of ext_oneapi_unknown

f70f49d

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

alexanderfle requested review from vladimirlaz and romanovvlad December 13, 2021 12:19

alexanderfle added 2 commits December 14, 2021 17:02

Merge branch 'sycl' into discard_events_implementation

fe6abbf

[SYCL] Merge 'sycl' into discard_events_implementation

6617fc7

Signed-off-by: Alexander Flegontov <alexander.flegontov@intel.com>

romanovvlad reviewed Dec 14, 2021

View reviewed changes

vladimirlaz approved these changes Dec 15, 2021

View reviewed changes

romanovvlad merged commit 9542e28 into intel:sycl Dec 15, 2021

alexanderfle mentioned this pull request Feb 2, 2022

[SYCL][DOC] Move discard queue events to supported #5452

Merged

		// The presence of support for DiscardEvents from the queue
		const bool MHasDiscardEventsSupport;

		if (info::event_command_status::ext_oneapi_unknown ==
		Event.get_info<info::event::command_execution_status>()) {

	"waitInternal method cannot be used for an discarded event.");
	"waitInternal method cannot be used for a discarded event.");

[SYCL] Implementation of discard_events #5026

[SYCL] Implementation of discard_events #5026

Uh oh!

Conversation

alexanderfle commented Nov 24, 2021

Uh oh!

alexanderfle commented Nov 26, 2021

Uh oh!

alexanderfle commented Dec 3, 2021

Uh oh!

alexanderfle commented Dec 7, 2021

Uh oh!

vladimirlaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad Dec 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romanovvlad Dec 10, 2021 •

edited

Loading