Skip to content

[SYCL][L0] code restructuring for keeping command-list related info together #4248

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

Jump to bottom
Merged
merged 8 commits into from
Aug 6, 2021
Merged

[SYCL][L0] code restructuring for keeping command-list related info together #4248

merged 8 commits into from
Aug 6, 2021

Conversation

smaslov-intel
Copy link
Contributor

The changes are mostly NFC, except that I replaced ZeOpenCommandListSize with CommandList->EventList.size(), which should be identical in the new design.

Signed-off-by: Sergey V Maslov [email protected]

@smaslov-intel smaslov-intel requested a review from asudarsa August 4, 2021 01:06
@smaslov-intel
Copy link
Contributor Author

/summary:run

@asudarsa
Copy link
Contributor

asudarsa commented Aug 4, 2021

It might be a good idea to wait for PR #4221 to land and then rework your changes here as there are some overlapping sections (ZeCommandListFenceMap implementation has been changed a bit in #4221).

@smaslov-intel
Copy link
Contributor Author

It might be a good idea to wait for PR #4221 to land and then rework your changes here as there are some overlapping sections (ZeCommandListFenceMap implementation has been changed a bit in #4221).

Yes, I will take care of the merge.

asudarsa
asudarsa reviewed Aug 4, 2021
View reviewed changes
asudarsa
asudarsa reviewed Aug 5, 2021
View reviewed changes
asudarsa
asudarsa reviewed Aug 5, 2021
View reviewed changes
asudarsa
asudarsa reviewed Aug 5, 2021
View reviewed changes
@smaslov-intel
[SYCL][L0] code restructuring for keeping command-list related info t…
c17e374 
…ogether

Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
fix segfaults
8b7b40b 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
clang-format
6c754cb 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
fix ==
277dc9c 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
fix test fails
6a91b57 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
clang-format
ce4f781 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
fix batching
07993cb 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
rebase
6fef6da 
Signed-off-by: Sergey V Maslov <[email protected]>
@smaslov-intel
Copy link
Contributor Author

It might be a good idea to wait for PR #4221 to land and then rework your changes here as there are some overlapping sections (ZeCommandListFenceMap implementation has been changed a bit in #4221).

Yes, I will take care of the merge.

Merged, please review.

@smaslov-intel smaslov-intel requested a review from asudarsa August 5, 2021 19:47
asudarsa
asudarsa reviewed Aug 6, 2021
View reviewed changes
asudarsa
asudarsa approved these changes Aug 6, 2021
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick. Otherwise, it looks good to me. Thanks

@bader bader merged commit 7e22445 into intel:sycl Aug 6, 2021
@bader
Copy link
Contributor

bader commented Aug 6, 2021

@smaslov-intel, this patch break the build with clang (aka "self-build"). Please, fix ASAP.
See https://github.com/intel/llvm/runs/3262011421 for the full log.

/home/runner/work/llvm/llvm/src/sycl/plugins/level_zero/pi_level_zero.hpp:346:15: error: anonymous non-C-compatible type given name for linkage purposes by typedef declaration; add a tag name here [-Werror,-Wnon-c-typedef-for-linkage]
typedef struct {
^
pi_command_list_info_t
/home/runner/work/llvm/llvm/src/sycl/plugins/level_zero/pi_level_zero.hpp:348:28: note: type is not C-compatible due to this default member initializer
ze_fence_handle_t ZeFence{nullptr};
^~~~~~~~~
/home/runner/work/llvm/llvm/src/sycl/plugins/level_zero/pi_level_zero.hpp:372:3: note: type is given name 'pi_command_list_info_t' for linkage purposes by this typedef declaration
} pi_command_list_info_t;
^
1 error generated.

@smaslov-intel
Copy link
Contributor Author

@smaslov-intel, this patch break the build with clang (aka "self-build"). Please, fix ASAP.

Fix is in #4286

al42and added a commit to al42and/llvm that referenced this pull request Aug 17, 2021
@al42and
[SYCL][L0] Always set CommandList in _pi_context::getAvailableCommand…
90970ed 
…List

When adding a command list from the cache to the map, we were not returning
the newly-inserted element via `CommandList` reference.

Thus, the _pi_context::getAvailableCommandList function was returning
`PI_SUCCESS`, but `CommandList` was left uninitialized, leading to a
segfault when the command list is used later (in my case, in `enqueueMemFillHelper`).

Introduced in PR intel#4248.
@al42and al42and mentioned this pull request Aug 17, 2021
Merged
bader pushed a commit that referenced this pull request Aug 18, 2021
@al42and
[SYCL][L0] Always set CommandList in _pi_context::getAvailableCommand…
cb2265b 
…List (#4359)

When adding a command list from the cache to the map, we were not returning
the newly-inserted element via `CommandList` reference.

Thus, the _pi_context::getAvailableCommandList function was returning
`PI_SUCCESS`, but `CommandList` was left uninitialized, leading to a
segfault when the command list is used later (in my case, in `enqueueMemFillHelper`).

Introduced in PR #4248.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudarsa asudarsa asudarsa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smaslov-intel @asudarsa @bader