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

[SYCL] Ignore usm prefetch dummy flag #4568

Merged
merged 5 commits into from
Sep 15, 2021

Conversation

AidanBeltonS
Copy link
Contributor

Patch removes assertions in pi_cuda, pi_hip, and pi_level_zero which fail based upon the flag given to the function.
prefetch_usm function now does not pass flag to PI.
The flag is a placeholder and should be ignored.

This is a follow up PR from issue #4467

@steffenlarsen
Copy link
Contributor

Instead of changing the migration flags to 0, it would be better to adopt the assertion this PR removes from level_zero. PI_USM_MIGRATION_TBD0 is there to indicate that more flags are likely incoming, so when flags are introduced the assertion would fail and you'd know quickly that the backend needs fixing. Arguably since it is a flag, 0 should also specify that no adjustments are requested so it should serve approximately the same purpose, but at least the name PI_USM_MIGRATION_TBD0 makes the future intention clearer.

@AidanBeltonS
Copy link
Contributor Author

I think it makes more sense to pass 0. As it expresses that there is no flag, so no action is needed.
As for the asserts I agree that it is desirable to move to the pi_level_zero assert as new flags would then fail.

@steffenlarsen
Copy link
Contributor

I think it makes more sense to pass 0. As it expresses that there is no flag, so no action is needed.

Fair point. Then PI CUDA and PI HIP can just keep their asserts.

@AidanBeltonS
Copy link
Contributor Author

Then PI CUDA and PI HIP can just keep their asserts

Would it not make sense to move all PI's over to one sort of assert? As all ignore the flag in the same way.
So all should go to either use the level_zero or cuda/hip assert.

@steffenlarsen
Copy link
Contributor

Would it not make sense to move all PI's over to one sort of assert? As all ignore the flag in the same way.
So all should go to either use the level_zero or cuda/hip assert.

I mostly intended to make it clear that there should be some sort of check at least, where the easiest option would be to simply restore the events. PI_ASSERT is defined in pi_level_zero.cpp as a return of the specified error if the condition does not evaluate to true, so when/if a flag is added in the future it would show up in both release and debug builds, which I agree is likely the better option.

@AidanBeltonS
Copy link
Contributor Author

Ahh okay, sorry I misunderstood. Thank you for clarifying.

@steffenlarsen
Copy link
Contributor

Ahh okay, sorry I misunderstood. Thank you for clarifying.

No worries. I could have been clearer to begin with. 😄

@@ -4589,22 +4589,22 @@ pi_result cuda_piextUSMEnqueueMemcpy(pi_queue queue, pi_bool blocking,
return result;
}

// flags is currently ignored as it is a placeholder for future features
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor, but it is not "ignored" anymore, so maybe the comment should be inside the function and be adapted to reflect the new behavior for unrecognized flags.

pi_result cuda_piextUSMEnqueuePrefetch(pi_queue queue, const void *ptr,
size_t size,
pi_usm_migration_flags flags,
pi_uint32 num_events_in_waitlist,
const pi_event *events_waitlist,
pi_event *event) {

if (!(flags & ~PI_USM_MIGRATION_TBD0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we now expect flags to be 0, the condition here and in the other backends should be flags != 0. Note that level-zero negates the condition, since it is an assert, so its condition should be flags == 0 instead.

@steffenlarsen
Copy link
Contributor

Does SYCL/USM/prefetch.cpp in intel/llvm-test-suite pass for CUDA with these changes?

@AidanBeltonS
Copy link
Contributor Author

Does SYCL/USM/prefetch.cpp in intel/llvm-test-suite pass for CUDA with these changes?

Yes SYCL/USM/prefetch.cpp and SYCL/USM/dep_events.cpp both now pass. I'll create a PR to remove XFAIL: cuda from these after this merges.

steffenlarsen
steffenlarsen previously approved these changes Sep 14, 2021
@smaslov-intel
Copy link
Contributor

@AidanBeltonS , please also add comment to the definition of PI_USM_MIGRATION_TBD0 that it's the placeholder with no defined meaning at the moment.

@bader bader merged commit 6c6d902 into intel:sycl Sep 15, 2021
@bader
Copy link
Contributor

bader commented Sep 16, 2021

Does SYCL/USM/prefetch.cpp in intel/llvm-test-suite pass for CUDA with these changes?

Yes SYCL/USM/prefetch.cpp and SYCL/USM/dep_events.cpp both now pass. I'll create a PR to remove XFAIL: cuda from these after this merges.

@AidanBeltonS, eagerly looking forward for this to fix llvm-test-suite.

@AidanBeltonS
Copy link
Contributor Author

llvm-test-suite PR has been created
intel/llvm-test-suite#464

alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Sep 18, 2021
* upstream/sycl: (36 commits)
  [SYCL] Add SYCL2020 target::device enumeration value (intel#4587)
  [SYCL][Doc] Update ITT instrumentation docs (intel#4503)
  [SYCL][L0] Make all L0 events have device-visibility (intel#4534)
  [SYCL] Updated Level-Zero backend spec according to SYCL 2020 standard (intel#4560)
  [SYCL] Add error_code support for SYCL 1.2.1 exception classes (intel#4574)
  [SYCL][CI] Provide --ci-defaults option for config script (intel#4583)
  [CI] Switch GitHub Actions to Ubuntu 20.04 (intel#4582)
  [SYCL][CUDA] Fix context clearing in PiCuda tests (intel#4483)
  [SYCL] Hide SYCL service kernels (intel#4519)
  [SYCL][L0] Fix mismatched ZE call count (intel#4559)
  [SYCL] Remove function pointers extension (intel#4459)
  [GitHub Actions] Uplift clang version in post-commit validation (intel#4581)
  [SYCL] Ignore usm prefetch dummy flag (intel#4568)
  [SYCL][Group algorithms] Add group sorting algorithms implementation (intel#4439)
  [SYCL] Resolve name clash with a user defined symbol (intel#4570)
  [clang-offload-wrapper] Do not create .tgtimg section with -emit-reg-funcs=0 (intel#4577)
  [SYCL][FPGA] Remove deprecated attribute functionality (intel#4532)
  [SYCL] Remove _class aliases (intel#4465)
  [SYCL][CUDA][HIP] Report every device in its own platform (intel#4571)
  [SYCL][L0] make_device shouldn't need platform as an input (intel#4561)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants