Skip to content

[SYCL]Remove warning about SYCL_EXTERNAL with pointers #2722

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 3 commits into from
Nov 5, 2020

Conversation

srividya-sundaram
Copy link
Contributor

Raw pointers for SYCL_EXTERNAL are prohibited without generic address space. Generic address space doesn't exist in 1.2.1 and is optional in 2020. Our implementation isn't spec compliant to 1.2.1 because we use generic address space (among other).
This diagnostic has no practical value to users. We aren't warning about something which has any impact for our implementation based on generic address space.

@srividya-sundaram srividya-sundaram marked this pull request as ready for review November 3, 2020 05:31
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

I don't have strong preference here, but this diagnostic was spec-defined behavior even though our implementation of address spaces is not SYCL 1.2.1 compliant. So, it would be great to hear feedback from @mkinsner and @rolandschulz here.

@srividya-sundaram
Copy link
Contributor Author

@Fznamznon This change was made based on @rolandschulz's input. I will share his comment below.
@rolandschulz Please feel free to provide your feedback on this change. Thanks!

Raw pointers for SYCL_EXTERNAL are prohibited without generic address space. Generic address space doesn't exist in 1.2.1 and is optional in 2020. Our implementation isn't spec compliant to 1.2.1 because we use generic address space (among other).

It seems useless to warn about a detail (SYCL_EXTERNAL with pointers) if the base functionality (pointers using inference rules vs generic address space) isn't implemented spec compliant and the reason for the spec rule is based on the assumption of the implementation.

And this diagnostic has no practical value to users. We aren't warning about something which has any impact for our implementation based on generic address space. And it isn't useful for portability to other implementation. Because this is only a portability concern if the other implementation doesn't support generic address space.

But in that case a useful warning needs to warn for any functionality which isn't supported without generic address space (using the SYCL inference rules instead). This is among other SYCL_EXTERNAL with raw pointer, assignment of raw pointers which can't be inferred (e.g. in conditionals), function ptrs with raw pointers. That warning would allow a user to target implementation which don't support generic address space. And as part of that warning its useful to warn for SYCL_EXTERNAL. But just warning for SYCL_EXTERNAL without also warning for all other features which depend on generic address space is useless.

@rolandschulz
Copy link
Contributor

The PR message contains one of my sentences:

This diagnostic has no practical value to users.

But it doesn't contain the other part:

[] a useful warning needs to warn for any functionality which isn't supported without generic address space

We might want to create a new issue to add such a useful warning and partially revert this commit and make it a part of such a new optional warning. I'm not sure whether users need that or would just use a different SYCL implementation without generic address space if they need to get diagnostic regarding features not available without generic address space.

Either way I think we want to use this commit for now.

rolandschulz
rolandschulz previously approved these changes Nov 3, 2020
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

LGTM

@srividya-sundaram
Copy link
Contributor Author

srividya-sundaram commented Nov 3, 2020

I don't see any test failures reported in buildbot/Lit_With_Cuda.
But I do see the following time out error:

command timed out: 1200 seconds without output running [b'python3', b'llvm_ci/intel/worker/tools/build.py', b'-n', b'3968', b'-b', b'pull/2722/head', b'-r', b'2722', b'-p', b'sycl', b'-s', b'configuration', b'-t', b'Release', b'--cuda', b'-P', b'intel/llvm', b'-m', b'Lit_With_Cuda', b'-e', b'c4324818656e725b3f4b538a0ba7d06e9f985917', b'-U', b'http://ci.llvm.intel.com:8010/#/builders/37/builds/3968'], attempting to kill
process killed by signal 9
program finished with exit code -1

I tried retriggering the build a couple of times but still get the above failure.
@bader @romanovvlad Any idea if this is a known infrastructure issue?

Update: The Cuda buildbot has passed this time.

@romanovvlad romanovvlad merged commit b4a3f03 into intel:sycl Nov 5, 2020
@srividya-sundaram srividya-sundaram deleted the remove-sycl-attr-warning branch November 5, 2020 17:30
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Nov 6, 2020
* upstream/sycl:
  [SYCL] Move tests with dependencies to on-device directory (intel#2732)
  [SYCL][Test] Remove leftovers for FPGA archives (intel#2735)
  [SYCL][NFC] Extend ABI tests to cover device code (intel#2725)
  [SYCL] Fix link to ESIMD tests (intel#2736)
  Added the SYCL_INTEL_mem_channel_property extension specification (intel#2688)
  [SYCL] Add support for new FPGA loop attribute nofusion (intel#2715)
  [SYCL] Remove host-task-dependency test added to llvm-test-suite (intel#2720)
  [SYCL] Remove warning about SYCL_EXTERNAL with pointers (intel#2722)
  [SYCL] dot_product support. (intel#2609)
  [SYCL][PI][L0] Fix a problem with kernels and programs destruction while they can be used (intel#2710)
  [SYCL] Fix the check for read-only host pointer during memobj creation (intel#2697)
@bader
Copy link
Contributor

bader commented Nov 9, 2020

I don't see any test failures reported in buildbot/Lit_With_Cuda.
But I do see the following time out error:

command timed out: 1200 seconds without output running [b'python3', b'llvm_ci/intel/worker/tools/build.py', b'-n', b'3968', b'-b', b'pull/2722/head', b'-r', b'2722', b'-p', b'sycl', b'-s', b'configuration', b'-t', b'Release', b'--cuda', b'-P', b'intel/llvm', b'-m', b'Lit_With_Cuda', b'-e', b'c4324818656e725b3f4b538a0ba7d06e9f985917', b'-U', b'http://ci.llvm.intel.com:8010/#/builders/37/builds/3968'], attempting to kill
process killed by signal 9
program finished with exit code -1

I tried retriggering the build a couple of times but still get the above failure.
@bader @romanovvlad Any idea if this is a known infrastructure issue?

Update: The Cuda buildbot has passed this time.

@dm-vodopyanov and @alexbatashev were looking into this issue in the past. It's a known issue, but it doesn't look like "infrastructure" issue.

@srividya-sundaram
Copy link
Contributor Author

@bader Thanks for clarifying that it is not an "infrastructure" issue. IIRC, I saw this issue in one other PR at least.
@dm-vodopyanov , @alexbatashev Can you please share if this issue is fixed?

@alexbatashev
Copy link
Contributor

@bader Thanks for clarifying that it is not an "infrastructure" issue. IIRC, I saw this issue in one other PR at least.
@dm-vodopyanov , @alexbatashev Can you please share if this issue is fixed?

At the moment, the root cause of this issue is unknown.

jsji pushed a commit that referenced this pull request Sep 21, 2024
…ure (#2722)

Fix issue #2721: Incorrect translation of calls to a builtin that returns a structure: create just one load, and account for a special case when Translator prepared a well-known pattern (store/load) for itself.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@dc1221cd83e67ef
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.

Add a useful warning to warn for any functionality which isn't supported without generic address space
7 participants