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] Enable UMF tracking by default #1071

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Conversation

igchor
Copy link
Member

@igchor igchor commented Nov 13, 2023

It is needed by adapters's urUSMFree implementations where we rely on umfPoolByPtr (which always returns null if tracking is disabled).

@kbenzie should I also make a PR to the main branch? We plan to move the UMF to a separate repo anyway and I expect this option (UMF_ENABLE_POOL_TRACKING) to not be publicly exposed to UR users anymore.

@igchor igchor requested a review from a team as a code owner November 13, 2023 16:45
@igchor igchor force-pushed the tracking branch 2 times, most recently from 8b3a8a4 to c13bb66 Compare November 14, 2023 20:32
@igchor
Copy link
Member Author

igchor commented Nov 14, 2023

I think this is ready to be merged. The failing tests turned out to just be tests that unexpectedly succeeded (most of the CUDA and L0 tests with UsePoolEnabled work after this change).

@lukaszstolarczuk
Copy link
Contributor

At this moment, if someone sets UMF_ENABLE_POOL_TRACKING manually to OFF, these tests will fail. Do we expect anyone to do that? Perhaps we should remove this option from CMake, after all?

@igchor
Copy link
Member Author

igchor commented Nov 16, 2023

@lukaszstolarczuk you are right, but I think I would actually prefer to do this after we merge / cherry-pick this change. Removing the option would also mean that we can remove some of the existing code so the change would be bigger.

@jandres742
Copy link

Fixes to test-usm in L0 in #1105 depending on this patch.

@jandres742
Copy link

@igchor :
is there anything extra to do on this PR, or could we ask @kbenzie to move it to "ready to merge" ?

@kbenzie
Copy link
Contributor

kbenzie commented Nov 28, 2023

@kbenzie should I also make a PR to the main branch? We plan to move the UMF to a separate repo anyway and I expect this option (UMF_ENABLE_POOL_TRACKING) to not be publicly exposed to UR users anymore.

I think we can just target adapters for now. I'm hoping we can merged adapters into main branch soon and then stop using the adapters branch so this problem will go away.

@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Nov 28, 2023
@kbenzie
Copy link
Contributor

kbenzie commented Nov 28, 2023

@igchor :
is there anything extra to do on this PR, or could we ask @kbenzie to move it to "ready to merge" ?

Might be prudent to rebase on top of adapters to make sure its working with the latest set of changes.

@kbenzie kbenzie added the v0.8.x Include in the v0.8.x release label Nov 28, 2023
@igchor
Copy link
Member Author

igchor commented Nov 28, 2023

@igchor :
is there anything extra to do on this PR, or could we ask @kbenzie to move it to "ready to merge" ?

Might be prudent to rebase on top of adapters to make sure its working with the latest set of changes.

Done. Do we want to wait until changes from #1084 are cherry-picked? If we merge this first, there might be some extra Windows failures.

@kbenzie
Copy link
Contributor

kbenzie commented Nov 29, 2023

Done. Do we want to wait until changes from #1084 are cherry-picked? If we merge this first, there might be some extra Windows failures.

If that's a dependency, then yes.

@kbenzie
Copy link
Contributor

kbenzie commented Nov 29, 2023

Done. Do we want to wait until changes from #1084 are cherry-picked? If we merge this first, there might be some extra Windows failures.

If that's a dependency, then yes.

This will be included in #1072 which we'll aiming to merged to adapters next.

@igchor igchor force-pushed the tracking branch 2 times, most recently from eca0bfc to 07ca188 Compare December 1, 2023 18:26
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (fe2735a) 15.79% compared to head (3276f70) 15.79%.
Report is 4 commits behind head on adapters.

Files Patch % Lines
test/conformance/testing/include/uur/fixtures.h 0.00% 15 Missing ⚠️
test/conformance/kernel/urKernelSetArgSampler.cpp 0.00% 4 Missing ⚠️
...ance/kernel/urKernelSetSpecializationConstants.cpp 0.00% 2 Missing ⚠️
test/conformance/kernel/urKernelSetArgPointer.cpp 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##           adapters    #1071      +/-   ##
============================================
- Coverage     15.79%   15.79%   -0.01%     
============================================
  Files           223      223              
  Lines         31351    31365      +14     
  Branches       3511     3511              
============================================
+ Hits           4951     4953       +2     
- Misses        26349    26361      +12     
  Partials         51       51              

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

@kbenzie
Copy link
Contributor

kbenzie commented Dec 4, 2023

@igchor any idea why the checks failed?

It is needed by adapters's urUSMFree implementations
where we rely on umfPoolByPtr (which always returns null
if tracking is disabled).
@igchor
Copy link
Member Author

igchor commented Dec 4, 2023

@kbenzie there was a conflict in test/conformance/kernel/kernel_adapter_level_zero.match. It should be fixed now and there's one less test failing for L0 kernel test.

@kbenzie
Copy link
Contributor

kbenzie commented Dec 5, 2023

Thanks @igchor

@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:50
@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 kbenzie merged commit cf92be2 into oneapi-src:main Dec 6, 2023
51 checks passed
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Dec 6, 2023
[UR] Enable UMF tracking by default
@kbenzie kbenzie mentioned this pull request Dec 6, 2023
13 tasks
@igchor igchor deleted the tracking branch December 6, 2023 17:13
kbenzie added a commit to kbenzie/llvm that referenced this pull request Dec 6, 2023
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Dec 8, 2023
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Dec 15, 2023
[UR] Enable UMF tracking by default
callumfare pushed a commit to kbenzie/llvm that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge 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