[SYCL][ESIMD][EMU] Queue/Event handling change for esimd_cpu #4201

dongkyunahn-intel · 2021-07-28T01:08:42Z

Supporting only in-order queue for pi_esimd_cpu
Event creation is changed to fail-safe one. Imported from pi_cuda.cpp

- Including esimd_cpu in work-around for revised queue::wait() logic same as level-zero, introduced in PR#4044 - Returning 'false' from ExecCGCommand:: producesPiEvent() predicate for esimd_cpu as no effective event is created during esimd_cpu kernel execution

romanovvlad · 2021-07-28T09:11:02Z

Including esimd_cpu in work-around for revised queue::wait() logic
same as level-zero, introduced in PR#4044
Returning 'false' from ExecCGCommand:: producesPiEvent() predicate...

Could you please clarify why this is necessary? What is the problem with "dummy" events if all operations are completed during piEnqueue*?

Tagging @sergey-semenov

dongkyunahn-intel · 2021-07-28T19:29:28Z

Some ESIMD samples from intel/llvm-test-suite fails without this change - e.g. vadd_1d.

What is the problem with "dummy" events if all operations are completed during piEnqueue*?

'DummyEvents' are still observed by piEventsWait. In order to completely eliminate events from piEnqueueMemBufferRead and piEnqueueMemImageRead done by CPU, the event returned from those PI_APIs must be nullptr (#4011 (comment)). Even if nullptr is returned from those APIs, some ESIMD samples are still failing - e.g. accessor, PrefixSum.

Meanwhile, I have used ESIMD samples from intel/llvm-test-suite for enabling ESIMD_CPU support. So far, none of those samples has invoked piEventCreate function (this is why piEventCreate in PR#4011 is still empty). I would need an ESIMD sample actively using events for fully enabling this event handling.

romanovvlad · 2021-08-02T18:31:44Z

Some ESIMD samples from intel/llvm-test-suite fails without this change - e.g. vadd_1d.

What is the problem with "dummy" events if all operations are completed during piEnqueue*?

'DummyEvents' are still observed by piEventsWait. In order to completely eliminate events from piEnqueueMemBufferRead and piEnqueueMemImageRead done by CPU, the event returned from those PI_APIs must be nullptr (#4011 (comment)). Even if nullptr is returned from those APIs, some ESIMD samples are still failing - e.g. accessor, PrefixSum.

Meanwhile, I have used ESIMD samples from intel/llvm-test-suite for enabling ESIMD_CPU support. So far, none of those samples has invoked piEventCreate function (this is why piEventCreate in PR#4011 is still empty). I would need an ESIMD sample actively using events for fully enabling this event handling.

Still not sure I understand. Can we return "dummy" events from piEnqueue* APIs and then just ignore them in all PI APIs that take events?

dongkyunahn-intel · 2021-08-03T06:24:31Z

Still not sure I understand. Can we return "dummy" events from piEnqueue* APIs and then just ignore them in all PI APIs that take events?

piEnqueue* APIs and piEventsWait are implemented as your description. Some kernels are still failing without patch from this PR. I was investigating the failures after making kernels work but still cannot understand why.

piEnqueueMemBufferRead- https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1198

piEnqueueMemImageRead - https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1293

piEventsWait - https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1078

romanovvlad · 2021-08-03T10:49:53Z

Still not sure I understand. Can we return "dummy" events from piEnqueue* APIs and then just ignore them in all PI APIs that take events?

piEnqueue* APIs and piEventsWait are implemented as your description. Some kernels are still failing without patch from this PR. I was investigating the failures after making kernels work but still cannot understand why.

piEnqueueMemBufferRead- https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1198

piEnqueueMemImageRead - https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1293

piEventsWait - https://github.com/intel/llvm/pull/4011/files#diff-d023d374bba88549a2b0637e35fb5d6d463b1fc885e86460bec7a563da17fa91R1078

I think we need to find the root cause before committing the patch.

dongkyunahn-intel · 2021-08-19T17:00:15Z

I think I found out solution. It has to be applied on top of the change set from PR#4011. As of August/19/2021, the PR is approved, but buildbot jobs fail due to missing packages required for online-building libva which is pre-requisite for CM. As soon as PR#4011 is merged into origin/sycl branch, I'll update this PR.

…eue_event_handling

romanovvlad · 2021-08-28T19:33:08Z

@dongkyunahn-intel

Returning 'false' from ExecCGCommand:: producesPiEvent() predicate
for esimd_cpu as no effective event is created during esimd_cpu kernel
execution

Could you please tell if we can create a dummy event during esimd_cpu kernel execution?

dongkyunahn-intel · 2021-08-30T04:41:01Z

Could you please tell if we can create a dummy event during esimd_cpu kernel execution?

Dummy event is already applied for piEnqueueMemBufferRead and piEnqueueMemImageRead in pi_esimd_cpu.cpp

llvm/sycl/plugins/esimd_cpu/pi_esimd_cpu.cpp

Line 1084 in bf7eb4c

if (EventList[i]->IsDummyEvent) {

llvm/sycl/plugins/esimd_cpu/pi_esimd_cpu.cpp

Line 1214 in bf7eb4c

(*Event)->IsDummyEvent = true;

llvm/sycl/plugins/esimd_cpu/pi_esimd_cpu.cpp

Line 1309 in bf7eb4c

(*Event)->IsDummyEvent = true;

romanovvlad · 2021-08-30T09:25:33Z

Dummy event is already applied for piEnqueueMemBufferRead and piEnqueueMemImageRead in pi_esimd_cpu.cpp

So, why we need to return false from ExecCGCommand::producesPiEvent() for esimd_cpu then?

- Event creation is imported from pi_cuda.cpp - Special handling for esimd_cpu in ExecCGCommand::producesPiEvent() (commands.cpp) is removed

dongkyunahn-intel · 2021-08-31T18:34:33Z

@romanovvlad, your suggestion worked with my fully working branch for ESIMD_CPU. After suggestion applied, only one file is changed.

Due to dependency on PR#4430, buildbots are failing during merge. I'll merge change sets from the PR to this PR when it is merged to origin/sycl branch.

dongkyunahn-intel · 2021-08-31T21:24:32Z

@romanovvlad / @kbobrovs ,

Can I apply change from this PR to PR#4430 (#4430)? As this change set is limited to one file, and by applying the change, we could prevent possible failures in PR#4430 caused by not having changes from this PR.

kbobrovs · 2021-08-31T22:07:12Z

@romanovvlad / @kbobrovs ,

Can I apply change from this PR to PR#4430 (#4430)? As this change set is limited to one file, and by applying the change, we could prevent possible failures in PR#4430 caused by not having changes from this PR.

Just curious - why not merge this PR first? I'm OK with uniting the two PRs.

dongkyunahn-intel · 2021-08-31T22:15:32Z

Just curious - why not merge this PR first? I'm OK with uniting the two PRs.

It still has dependency on PR#4430 (4011 previously) as it changes some PI_API implementation.

kbobrovs · 2021-08-31T22:41:38Z

sycl/plugins/esimd_cpu/pi_esimd_cpu.cpp

@@ -729,7 +736,9 @@ pi_result piQueueRelease(pi_queue Queue) {
 }

 pi_result piQueueFinish(pi_queue) {
-  DIE_NO_IMPLEMENTATION;
+  // TODO/FIXME : Only in-order queue is called for this API. Support


piQueueFinish must guarantee that all previously enqueued commands had completed. The fact that the queue is in-order does not guarantee that. In-order queue just means enqueued commands will execute in the enqueue order. Still, they can can execute asynchronously with the thread which enqueues the commands.

As far as I understand your current implementation, it is safe for piQueueFinish to be no-op just because all the commands like kernel execution or memory copy are blocking - enqueue of any command with ESIMD EMU plugin does not return until the command is actually finished.

If so, please change the comment.

Changed the comment.

kbobrovs · 2021-08-31T22:48:22Z

sycl/plugins/esimd_cpu/pi_esimd_cpu.cpp

+  std::unique_ptr<_pi_event> RetEv{nullptr};
+  if (Event) {
+    RetEv = std::unique_ptr<_pi_event>(new _pi_event());
+    RetEv->IsDummyEvent = true;


As I understand your current implementation, with all commands being blocking, all events returned from enqueue operations are effectively in "completed" state once control is returned back to the caller of the enqueue operation.

Given that I don't see the need in IsDummyEvent field.

Second, the above code is not needed, instead, the line 1218 should be '*Event = new _pi_event();' plus maybe setting the event to 'completed' state.

The same applies to other places where events are created.

@romanovvlad suggested this implementation from CUDA PI implementation from recent phone call. Also, he was fine with this 'DummyEvent' approach.

@romanovvlad suggested this implementation from CUDA PI implementation from recent phone call.

OK, now I see the point - unique_ptr is useful to automatically release the memory allocated by new _pi_even in case buf->CmBufferPtr->ReadSurface throws an exception. Disregard this my comment then.

Also, he was fine with this 'DummyEvent' approach.

DummyEvent seems is redundant, as I explained above, even though harmless. So I don't mind leaving it as is for now.

dongkyunahn-intel · 2021-09-01T21:01:25Z

As PR#4430 is almost complete without change from this PR, I'll keep this active and merge later.

…eue_event_handling

dongkyunahn-intel requested a review from a team as a code owner July 28, 2021 01:08

dongkyunahn-intel requested a review from romanovvlad July 28, 2021 01:08

clang-format error fix

c291db3

romanovvlad requested a review from kbobrovs July 28, 2021 09:06

Merge branch 'sycl' into esimdcpu_queue_event_handling

05e656b

Merge branch 'sycl' into esimdcpu_queue_event_handling

29d46df

dongkyunahn-intel added 3 commits August 27, 2021 11:21

Merge branch 'sycl' of https://github.com/intel/llvm into esimdcpu_qu…

349dc85

…eue_event_handling

Reverting changes applied in queue_impl.cpp

1987a64

Support only in-order queue for pi_esimd_cpu

e309c7f

dongkyunahn-intel requested a review from smaslov-intel as a code owner August 27, 2021 18:56

dongkyunahn-intel added 2 commits August 31, 2021 11:08

Update in event creation

65d39cd

- Event creation is imported from pi_cuda.cpp - Special handling for esimd_cpu in ExecCGCommand::producesPiEvent() (commands.cpp) is removed

clang-format error fix

605bd5e

kbobrovs reviewed Aug 31, 2021

View reviewed changes

Comment change per review comment

a26379a

romanovvlad approved these changes Sep 1, 2021

View reviewed changes

Merge branch 'sycl' of https://github.com/intel/llvm into esimdcpu_qu…

16b3de6

…eue_event_handling

dongkyunahn-intel requested review from kbobrovs and romanovvlad September 3, 2021 16:52

kbobrovs approved these changes Sep 3, 2021

View reviewed changes

smaslov-intel approved these changes Sep 3, 2021

View reviewed changes

kbobrovs merged commit 6d8abb9 into intel:sycl Sep 3, 2021

[SYCL][ESIMD][EMU] Queue/Event handling change for esimd_cpu #4201

[SYCL][ESIMD][EMU] Queue/Event handling change for esimd_cpu #4201

Uh oh!

Conversation

dongkyunahn-intel commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanovvlad commented Jul 28, 2021

Uh oh!

dongkyunahn-intel commented Jul 28, 2021

Uh oh!

romanovvlad commented Aug 2, 2021

Uh oh!

dongkyunahn-intel commented Aug 3, 2021

Uh oh!

romanovvlad commented Aug 3, 2021

Uh oh!

dongkyunahn-intel commented Aug 19, 2021

Uh oh!

romanovvlad commented Aug 28, 2021

Uh oh!

dongkyunahn-intel commented Aug 30, 2021

Uh oh!

romanovvlad commented Aug 30, 2021

Uh oh!

dongkyunahn-intel commented Aug 31, 2021

Uh oh!

dongkyunahn-intel commented Aug 31, 2021

Uh oh!

kbobrovs commented Aug 31, 2021

Uh oh!

dongkyunahn-intel commented Aug 31, 2021

Uh oh!

kbobrovs Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

dongkyunahn-intel Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Aug 31, 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!

dongkyunahn-intel Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Sep 1, 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!

dongkyunahn-intel commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dongkyunahn-intel commented Jul 28, 2021 •

edited

Loading

kbobrovs Aug 31, 2021 •

edited

Loading

kbobrovs Sep 1, 2021 •

edited

Loading

dongkyunahn-intel commented Sep 1, 2021 •

edited

Loading