Skip to content

[SYCL] Make context constructors explicit to avoid unintended conversions #1219

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

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

jbrodman
Copy link
Contributor

Signed-off-by: James Brodman [email protected]

@jbrodman jbrodman requested review from Pennycook and bader February 28, 2020 17:49
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

I agree with the change, but I'm wondering if we should also introduce a catch-all extension document for small deviations from the specification like this.

@keryell
Copy link
Contributor

keryell commented Feb 28, 2020

And also generally track this in the specification committee since often there happen to be specification bugs...

@jbrodman
Copy link
Contributor Author

@keryell Expect a spec PR. :)

@bader bader merged commit c220eb8 into intel:sycl Mar 2, 2020
@elizabethandrews
Copy link
Contributor

Should this be done for queue class as well? device to queue conversions cause crashes as well. I haven't verified this but I am pretty sure that a call cl::sycl::free(ptr_1k, device); will crash once this patch is merged in, due to the device to queue conversion.

alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 4, 2020
…ctor_tests

* origin/sycl: (32 commits)
  [SYCL] Fix circular reference between events and queues (intel#1226)
  [CI][Doc] Use SSH to deploy GitHub Pages (intel#1232)
  [SYCL][CUDA][Test] Testing for use of CUDA primary context (intel#1174)
  [SYCL] allow underscore symbol in temporary directory name
  [SYCL] Reject zero length arrays (intel#1153)
  [SYCL] Fix static code analyzis concerns (intel#1189)
  [SYCL] Add more details about the -fintelfpga option (intel#1218)
  [SYCL][CUDA] Select only NVPTX64 device binaries (intel#1223)
  [SYCL] Reverse max work-group size order (intel#1177)
  [SYCL][Doc] Add GroupAlgorithms extension (intel#1079)
  [SYCL] Fix SYCL internal enumerators conflict with user defined macro (intel#1188)
  [SYCL][CUDA] Fixes context release and unnamed context scope (intel#1207)
  [SYCL][CUDA] Fix context creation property parsing
  [CUDA][PI] clang-format pi.h
  [SYCL][CUDA] Handle the case of not having any CUDA device (intel#1212)
  [SYCL] Fix check-sycl-deploy target problems (intel#1165)
  [SYCL] Disable tests which take more than 5 minutes (intel#1220)
  [SYCL] Make context constructors explicit to avoid unintended conversions (intel#1219)
  [SYCL][NFC] Add clang-format configuration file for SYCL LIT tests (intel#1224)
  [SYCL] Fix command cleanup invoked from multiple threads (intel#1214)
  ...
@jbrodman
Copy link
Contributor Author

jbrodman commented Mar 9, 2020

Should this be done for queue class as well? device to queue conversions cause crashes as well. I haven't verified this but I am pretty sure that a call cl::sycl::free(ptr_1k, device); will crash once this patch is merged in, due to the device to queue conversion.

Probably. It would also be good to add a test.

@intel intel deleted a comment from elizabethandrews Mar 10, 2020
@elizabethandrews
Copy link
Contributor

Ok. Will do.

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.

6 participants