[SYCL][Graph] Fix bug when host-task is submitted to in-order queue #12433

mfrancepillois · 2024-01-18T12:33:31Z

When a host-task is submitted to in-order queue, dependency between this host-task and the successor is explicitly handled.
However, when we record an in-order queue, the recorded CG are not part of the regular in-order queue execution sequence.
But inter-CG dependencies are managed by the graph implementation.
This PR implements this point and ensures that recording an in-order does not impact the normal execution sequence.
Tests (e2e and unitest) have been added to check it.

…322) * [SYCL][Graph] Fix bug when host-task is submitted to in-order queue When a host-task is submitted to in-order queue, dependency between this host-task and the successor is explicitly handled. However, when we record an in-order queue, the recorded CG are not part of the regular in-order queue execution sequence. But inter-CG dependancies are managed by the graph implementation. This PR implements this point and ensures that recording an in-order does not impact the normal execution sequence. Tests (e2e and unitest) have been added to check it. Adds and Renames tests (e2e and unitests)

sycl/source/detail/queue_impl.cpp

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies.cpp

maarquitos14 · 2024-01-18T17:20:01Z

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memcpy.cpp

+  std::vector<T> Reference(Size);
+  std::memset(Reference.data(), 1, Size * sizeof(T));
+  for (size_t i = 0; i < Size; i++) {
+    Reference[i] += i;


I think you can check the output immediately after this statement in the same loop. Actually, you might not even need to create Reference at all. I think you can directly check assert(TestDataOut[i] = 1+i);.

You are right. Thanks for the suggestion. Reference has been removed.

I was too hasty in deleting Reference.
In fact, References were not equal to 1 but to 0x1010101 because the memset works with bytes.
I prefer to keep memset than a hard value, in case we change the size of variables in the future.
I thus updated the code to use a single Reference instead of an array.

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memset.cpp

sycl/unittests/Extensions/CommandGraph.cpp

…ged_dependencies.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memcpy.cpp

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memset.cpp

…ged_dependencies_memcpy.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

…ged_dependencies_memset.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

github-actions · 2024-01-19T16:53:59Z

✅ With the latest revision this PR passed the C/C++ code formatter.

maarquitos14

LGTM.

EwanC · 2024-01-23T12:02:28Z

tagging @intel/llvm-gatekeepers for merging

Fix memory leaks in newly added graph tests in #12433. Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>

mfrancepillois added 2 commits January 18, 2024 12:04

Updates unitests to be compliant with new node storages.

5be86cc

mfrancepillois had a problem deploying to WindowsCILock January 18, 2024 12:33 — with GitHub Actions Error

Removes REQUIRE from test-e2e

00edce1

mfrancepillois marked this pull request as ready for review January 18, 2024 12:40

mfrancepillois requested review from a team as code owners January 18, 2024 12:40

mfrancepillois requested a review from maarquitos14 January 18, 2024 12:40

mfrancepillois temporarily deployed to WindowsCILock January 18, 2024 12:40 — with GitHub Actions Inactive

mfrancepillois had a problem deploying to WindowsCILock January 18, 2024 13:05 — with GitHub Actions Failure

Handle the case of a barrier recorded in a inorder-queue

d8a434d

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 11:39 — with GitHub Actions Inactive

maarquitos14 reviewed Jan 19, 2024

View reviewed changes

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 12:04 — with GitHub Actions Inactive

mfrancepillois and others added 15 commits January 19, 2024 14:56

Update sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_mana…

a37e334

…ged_dependencies.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

f858bce

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

fb37577

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

d3f58fd

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

0a22909

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

7e215a3

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

26f5d20

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

a7afc39

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

8e58b59

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

11c1a2c

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

87810f9

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

252a1eb

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

76a5038

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

b22562c

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/unittests/Extensions/CommandGraph.cpp

d70ac0c

Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

mfrancepillois had a problem deploying to WindowsCILock January 19, 2024 15:15 — with GitHub Actions Error

Removes unnecessary variables + updates comments

15dcf99

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 15:51 — with GitHub Actions Inactive

maarquitos14 reviewed Jan 19, 2024

View reviewed changes

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memcpy.cpp Outdated Show resolved Hide resolved

sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_managed_dependencies_memset.cpp Outdated Show resolved Hide resolved

mfrancepillois had a problem deploying to WindowsCILock January 19, 2024 16:15 — with GitHub Actions Error

mfrancepillois and others added 2 commits January 19, 2024 16:49

Update sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_mana…

4afe7ee

…ged_dependencies_memcpy.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

Update sycl/test-e2e/Graph/RecordReplay/in_order_queue_with_host_mana…

2b824d6

…ged_dependencies_memset.cpp Co-authored-by: Marcos Maronas <maarquitos14@users.noreply.github.com>

clang-format

0e7ed5e

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 17:03 — with GitHub Actions Inactive

mfrancepillois had a problem deploying to WindowsCILock January 19, 2024 17:26 — with GitHub Actions Error

Fixes tests: adds single ref

c584c99

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 17:45 — with GitHub Actions Inactive

mfrancepillois temporarily deployed to WindowsCILock January 19, 2024 18:06 — with GitHub Actions Inactive

EwanC approved these changes Jan 22, 2024

View reviewed changes

maarquitos14 approved these changes Jan 22, 2024

View reviewed changes

sommerlukas merged commit 82a83f0 into intel:sycl Jan 23, 2024

maarquitos14 mentioned this pull request Jan 26, 2024

[SYCL][Graph] Fix memory leaks in new graph tests #12507

Merged

sommerlukas pushed a commit that referenced this pull request Jan 29, 2024

[SYCL][Graph] Fix memory leaks in new graph tests (#12507)

dbc0272

Fix memory leaks in newly added graph tests in #12433. Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>

EwanC mentioned this pull request Jan 31, 2024

[SYCL][Graph] host-task + in-order queue before recording graph node failure #11118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][Graph] Fix bug when host-task is submitted to in-order queue #12433

[SYCL][Graph] Fix bug when host-task is submitted to in-order queue #12433

Uh oh!

mfrancepillois commented Jan 18, 2024

Uh oh!

Uh oh!

Uh oh!

maarquitos14 Jan 18, 2024

Uh oh!

mfrancepillois Jan 19, 2024

Uh oh!

mfrancepillois Jan 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 19, 2024 •

edited

Loading

Uh oh!

maarquitos14 left a comment

Uh oh!

EwanC commented Jan 23, 2024

Uh oh!

Uh oh!

[SYCL][Graph] Fix bug when host-task is submitted to in-order queue #12433

[SYCL][Graph] Fix bug when host-task is submitted to in-order queue #12433

Uh oh!

Conversation

mfrancepillois commented Jan 18, 2024

Uh oh!

Uh oh!

Uh oh!

maarquitos14 Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

mfrancepillois Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

mfrancepillois Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maarquitos14 left a comment

Choose a reason for hiding this comment

Uh oh!

EwanC commented Jan 23, 2024

Uh oh!

Uh oh!

github-actions bot commented Jan 19, 2024 •

edited

Loading