Skip to content
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.

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

[UR][L0] Add several fixes to L0 adapter for test-usm #1105

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

jandres742
Copy link

@jandres742 jandres742 commented Nov 22, 2023

intel/llvm testing: intel/llvm#11980

return ReturnValue(Pool);
}
if (Pool->HostMemPool.get() == UMFPool) {
return ReturnValue(Pool->HostMemPool.get());
Copy link
Contributor

@kswiecicki kswiecicki Nov 22, 2023

Choose a reason for hiding this comment

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

This should return ReturnValue(Pool);

Copy link
Author

Choose a reason for hiding this comment

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

thanks @kswiecicki! fixed.

@@ -829,6 +884,10 @@ UR_APIEXPORT ur_result_t UR_APICALL urUSMPoolCreate(
try {
*Pool = reinterpret_cast<ur_usm_pool_handle_t>(
new ur_usm_pool_handle_t_(Context, PoolDesc));

std::shared_lock<ur_shared_mutex> ContextLock(Context->Mutex);
Context->UsmPoolHandles.push_back(*Pool);
Copy link
Contributor

Choose a reason for hiding this comment

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

Context->UsmPoolHandles should also be updated in urUSMPoolRelease.

Copy link
Author

Choose a reason for hiding this comment

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

thanks @kswiecicki. missed it. fixed.

@jandres742
Copy link
Author

There are some tests failing because we need to have UMF tracking. So we need to have this one merged #1071

@kbenzie kbenzie added the v0.8.x Include in the v0.8.x release label Nov 28, 2023
@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:47
@fabiomestre
Copy link
Contributor

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

@kbenzie
Copy link
Contributor

kbenzie commented Dec 6, 2023

There are some tests failing because we need to have UMF tracking. So we need to have this one merged #1071

I'm merged #1071 now so will rebase this on top of the main branch.

EDIT: I didn't notice the merge conflict, I'll let you resolve that as part of updating the branch @jandres742

@kbenzie
Copy link
Contributor

kbenzie commented Dec 7, 2023

I went ahead and rebased this on top of main.

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (95f9092) 15.73% compared to head (815a286) 15.74%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1105   +/-   ##
=======================================
  Coverage   15.73%   15.74%           
=======================================
  Files         223      223           
  Lines       31466    31466           
  Branches     3556     3556           
=======================================
+ Hits         4952     4953    +1     
+ Misses      26463    26462    -1     
  Partials       51       51           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kbenzie kbenzie marked this pull request as ready for review December 12, 2023 15:26
@kbenzie kbenzie requested review from a team as code owners December 12, 2023 15:26
@kbenzie
Copy link
Contributor

kbenzie commented Dec 12, 2023

I updated intel/llvm#11980 to point at the latest commit on this branch and its caused 121 failures. @nrspruit I think I've taken this one as far as I can. I'll need to hand that over to you.

@nrspruit
Copy link
Contributor

I updated intel/llvm#11980 to point at the latest commit on this branch and its caused 121 failures. @nrspruit I think I've taken this one as far as I can. I'll need to hand that over to you.

Thanks @kbenzie , I will be taking a look tomorrow to determine what is causing the failures.

@@ -187,8 +187,15 @@ static ur_result_t USMDeviceAllocImpl(void **ResultPtr,
ZeDesc.pNext = &RelaxedDesc;
}

ZE2UR_CALL(zeMemAllocDevice, (Context->ZeContext, &ZeDesc, Size, Alignment,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this macro was needed for the l0 api tracing enabled with UR_L0_DEBUG, that tracing it necessary for a lot of the sycl-e2e tests (they check for certain l0 api calls in the trace output). We should make these kinds of changes like this

ze_result_t ZeResult = ZE_CALL_NOCHECK(zeMemAllocDevice, (Context->ZeContext,
              &ZeDesc, Size, Alignment, Device->ZeDevice, ResultPtr));

instead

Jaime Arteaga and others added 3 commits December 13, 2023 18:26
Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
There is a discrepancy between Debug and Release build pass rates in the
USM test suite on Level Zero. Make those tests optional until the fix is
introduced.
Signed-off-by: Spruit, Neil R <neil.r.spruit@intel.com>
Signed-off-by: Spruit, Neil R <neil.r.spruit@intel.com>
Signed-off-by: Spruit, Neil R <neil.r.spruit@intel.com>
@kbenzie
Copy link
Contributor

kbenzie commented Dec 14, 2023

Looks like the SYCL E2E failures have been resolved so I'll merge this.

@kbenzie kbenzie merged commit 3e4d724 into oneapi-src:main Dec 14, 2023
51 checks passed
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Dec 14, 2023
[UR][L0] Add several fixes to L0 adapter for test-usm
sergey-semenov pushed a commit to intel/llvm that referenced this pull request Dec 14, 2023
Corresponding UR chanages
oneapi-src/unified-runtime#1105.

---------

Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Dec 15, 2023
[UR][L0] Add several fixes to L0 adapter for test-usm
callumfare pushed a commit to kbenzie/llvm that referenced this pull request Dec 18, 2023
Corresponding UR chanages
oneapi-src/unified-runtime#1105.

---------

Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.8.x Include in the v0.8.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants