Skip to content

[SYCL] Fix assertion for sycl keyword using without -fsycl option. #445

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
Aug 7, 2019

Conversation

jyu2-git
Copy link
Contributor

@jyu2-git jyu2-git commented Aug 1, 2019

This is to fix assertion when clang compile C++ head file without -fsycl

The problem is when __pipe (one of sycl keyworld) used without
-fsycl
option, the __pipe should not be keyword and keyword status should be
KS_Disable. But it is wrongly set to KS_Future.

The root problem is the KEYSYCL is add to KEYALLCXX:

KEYALLCXX = KEYSYCL | KEYCXX | KEYCXX11 | KEYCXX2A

It seems, we want KEYSYCL to be part of feature of C++. But which C++
should belong: C++11 or C++2a? (for example KEYCX2A... vs concept)

For now we should just use -fsycl option to control this.

Signed-off-by: Yu jennifer.yu@intel.com

erichkeane
erichkeane previously approved these changes Aug 1, 2019
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

I looked into this as well, this is correct.

@bader
Copy link
Contributor

bader commented Aug 1, 2019

__constant is not SYCL keyword after #358.

@erichkeane
Copy link
Contributor

This problem isn't limited to __constant. It'll happen with anything defined with KEYSYCL. __read_only, __write_only, and __pipe have the same problem. @jyu2-git you likely need to just update the test to use one of those other keywords instead.

@jyu2-git
Copy link
Contributor Author

jyu2-git commented Aug 1, 2019

I don't know if I have updated source. But it seems after 358, we don't defined any KEYSYCL keyword in TokenKinds.def. If that is the case, we need remove KEYSYCL for all use. If not, we still need to remove KEYSYCL form KEYALLCXX

@bader
Copy link
Contributor

bader commented Aug 1, 2019

I think __pipe should still be a keyword.

@bader
Copy link
Contributor

bader commented Aug 1, 2019

Could you also add [SYCL] tag to the commit message title, please?

@jyu2-git
Copy link
Contributor Author

jyu2-git commented Aug 1, 2019

Thanks I will add by end of day. The machine I am using currently used by other. So can not do now.

This is to fix assertion when clang compile C++ head file without -fsycl

The problem is when __constant (one of sycl keyworld) used without
-fsycl
option, the __contant should not be keyword and keyword status should be
KS_Disable.  But it is wrongly set to KS_Future.

The root problem is the KEYSYCL is add to KEYALLCXX:

KEYALLCXX = KEYSYCL | KEYCXX | KEYCXX11 | KEYCXX2A

It seems, we want KEYSYCL to be part of feature of C++.  But which C++
should belong: C++11 or C++2a?  (for example KEYCX2A... vs concept)

For now we should just use -fsycl option to control this.

Signed-off-by: Yu <jennifer.yu@intel.com>
@jyu2-git jyu2-git force-pushed the private/jyu2-git/syclkeyword branch from f44646c to f74132e Compare August 1, 2019 23:01
@jyu2-git jyu2-git changed the title [CLANG] Fix assertion for sycl keyword using without -fsycl option. [SYCL] Fix assertion for sycl keyword using without -fsycl option. Aug 2, 2019
@keryell
Copy link
Contributor

keryell commented Aug 3, 2019

None of these are actual SYCL keywords. They are just implementation details.
I have the feeling that most of them could be just replaced by some __attribute__((annotate("sycl::pipe"))) or whatever.

@jyu2-git
Copy link
Contributor Author

jyu2-git commented Aug 5, 2019

None of these are actual SYCL keywords. They are just implementation details.
I have the feeling that most of them could be just replaced by some __attribute__((annotate("sycl::pipe"))) or whatever.

Decision is needed from sycl team.

@agozillon
Copy link
Contributor

None of these are actual SYCL keywords. They are just implementation details.
I have the feeling that most of them could be just replaced by some __attribute__((annotate("sycl::pipe"))) or whatever.

Decision is needed from sycl team.

I think (but could be wrong) @keryell was just giving an opinion for some interesting future direction as I imagine it's outside of the scope of this pull request probably. As I think the annotate attribute might need some tweaking to be fully usable in all contexts; at least from my experience with it anyway. And I imagine some of the code gen/other components relating to the __pipe keyword would have to change.

But having annotate working nicely would be pretty useful for passing arbitrary data down to backends/opt passes and could be useful to make some generic way to express SYCL extensions/attributes. So some discussion on it from the SYCL team may be interesting!

@jyu2-git
Copy link
Contributor Author

jyu2-git commented Aug 6, 2019

Thanks @agozillon. Should we get this in for now? Thanks. Jennifer

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM.
Minor comment.

@bader
Copy link
Contributor

bader commented Aug 7, 2019

Please, update the description. __constant is not a SYCL keyword, so it's a bad example.

Also it might be useful to test that clang emits diagnostic if we try to use __pipe, __read_only, etc. as keywords.

@bader bader requested a review from erichkeane August 7, 2019 11:17
@jyu2-git
Copy link
Contributor Author

jyu2-git commented Aug 7, 2019

Thanks all, update comment

@bader bader merged commit a25e0e8 into intel:sycl Aug 7, 2019
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023
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.

5 participants