Skip to content

Conversation

@igchor
Copy link
Contributor

@igchor igchor commented Jul 24, 2024

No description provided.

@igchor igchor requested a review from a team as a code owner July 24, 2024 22:15
@github-actions github-actions bot added the level-zero L0 adapter specific issues label Jul 24, 2024
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

@igchor you made more changes to this code in #1894. do you still want this to be merged separately?

@igchor igchor force-pushed the event_no_return branch from 561c0b6 to 319388a Compare July 31, 2024 15:52
@igchor
Copy link
Contributor Author

igchor commented Jul 31, 2024

@igchor you made more changes to this code in #1894. do you still want this to be merged separately?

I think we could merge it separately, extra changes in the other PR are due to renaming and implementation of release() and retain() but that should be in a separate commit anywy

@igchor igchor requested a review from a team as a code owner August 6, 2024 16:19
@github-actions github-actions bot added the ci/cd Continuous integration/devliery label Aug 6, 2024
@igchor igchor changed the title [L0 v2] do not return ze_event_handle_t to the cache [L0 v2] event improvements Aug 6, 2024

ZE2UR_CALL_THROWS(zeEventPoolCreate,
(context->ZeContext, &desc, 1,
(context->hContext, &desc, 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have getters for ze handles. But we can do that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agree. I actually have the getters implemented already on a branch with enqueue implementation because we need them to be able to reuse some code from legacy adapter (like program.cpp)

@igchor igchor force-pushed the event_no_return branch 2 times, most recently from 59b6314 to e532d78 Compare August 6, 2024 17:33
@igchor igchor force-pushed the event_no_return branch 3 times, most recently from c884715 to 364dcce Compare August 6, 2024 23:09
@igchor igchor force-pushed the event_no_return branch 3 times, most recently from 21a30e6 to 4b30c1a Compare August 7, 2024 16:19
@igchor igchor requested a review from a team as a code owner August 7, 2024 16:53
@github-actions github-actions bot added the conformance Conformance test suite issues. label Aug 7, 2024
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

It looks like there are two changes in this PR, an update to GitHub workflows to run the V2 tests, and a change to the v2 event implementation. Should these be separate PRs?

@igchor
Copy link
Contributor Author

igchor commented Aug 8, 2024

It looks like there are two changes in this PR, an update to GitHub workflows to run the V2 tests, and a change to the v2 event implementation. Should these be separate PRs?

By mistake, my previous PR disabled the building L0 v2 adapter on CI, and this PR enables the build back. The improvements also contain a compilation fix, which is required for the build to pass. I could split it, but then the changes wouldn't be tested on the PR.

Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

command-buffer match file changes LGTM

@igchor igchor merged commit 89dc3a5 into oneapi-src:main Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd Continuous integration/devliery conformance Conformance test suite issues. level-zero L0 adapter specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants