Page MenuHomePhabricator

Introduce SYCL 2020 mode
ClosedPublic

Authored by aaron.ballman on May 11 2021, 10:56 AM.

Details

Summary

Currently, we have support for SYCL 1.2.1 (also known as SYCL 2017). This patch introduces the start of support for SYCL 2020 mode, which is the latest SYCL standard available at (https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html). This sets the default SYCL to be 2020 in the driver, and introduces the notion of a "default" version (set to 2020) when cc1 is in SYCL mode but there was no explicit -sycl-std= specified on the command line.

Diff Detail

Event Timeline

aaron.ballman created this revision.May 11 2021, 10:56 AM
aaron.ballman requested review of this revision.May 11 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 10:56 AM

LGTM, but want others to look first.

clang/lib/Frontend/InitPreprocessor.cpp
481

This seems very related, but perhaps a part of a different patch? I think I'm probably OK bringing this in during this patch, but it isn't strictly related to adding 2020 support.

aaron.ballman added inline comments.May 11 2021, 11:05 AM
clang/lib/Frontend/InitPreprocessor.cpp
481

Oops, I'll remove that one, you're right that it's unrelated. Thanks!

clang/test/Preprocessor/sycl-macro.cpp
13

This change is unrelated as well.

Remove some accidental additional changes for SYCL 2017.

tschuett added inline comments.
clang/include/clang/Basic/LangOptions.h
130

Do you want to change it to a scoped enum or will this cause major issues? ClangABI is a scoped enum?!?

aaron.ballman added inline comments.May 11 2021, 11:59 AM
clang/include/clang/Basic/LangOptions.h
130

Given that it's already scoped to LangOptions, I think a scoped enum adds more noise than anything. It'd make it awkward to name the actual enumerators due to using dates. I think we'd wind up needing to write LangOptions::SYCLMajorVersion::Ver2017 (or something along those lines), which doesn't seem like a huge win to me. WDYT?

No worries. I just wondered why only ClangABI is scoped ^^

dexonsmith added inline comments.May 11 2021, 12:15 PM
clang/include/clang/Basic/LangOptions.h
130

FWIW, I agree that a scoped enum wouldn't improve things here.

No worries. I just wondered why only ClangABI is scoped ^^

:-)

FWIW, I think it's because LangOptions::Ver1 would be hard to understand compared to LangOptions::ClangABI::Ver1, whereas in this case, LangOptions::SYCL_2020 is reasonably clear as to what's meant.

But there is no SYCLMinorVersion today. So enum class SYCLVersion would suffice.

erichkeane added a comment.EditedMay 11 2021, 12:29 PM

But there is no SYCLMinorVersion today. So enum class SYCLVersion would suffice.

I don't think the presence of the word 'Major' in Aaron's version above is the most offensive to me. While I think a good case can be made to make this just SYCLVersion (I don't think we have a minor version?), I think making this a scoped-enum is just going to end up with a worse experience. LangOptions::SYCL_2020 is equally as descriptive as LangOptions::SYCLVersion::Ver2020. And I'd say is even MORE descriptive, since Ver2020 has a very low information density.

The Ver in Ver2020 is redundant ::2020 would suffice.

The Ver in Ver2020 is redundant ::2020 would suffice.

That is not valid syntax.

My bad. My last comment:

LangOptions::SYCL::Ver2017
LangOptions::SYCL_2017

My bad. My last comment:

LangOptions::SYCL::Ver2017
LangOptions::SYCL_2017

To me, those convey the same amount of information, so the use of the scoped enum doesn't get us much (but it would mean we can't use LangOptions::SYCL for any other purpose in the future).

bader accepted this revision.May 18 2021, 6:04 AM

Sorry for the delay.
LGTM. Thanks!

This revision is now accepted and ready to land.May 18 2021, 6:04 AM
aaron.ballman closed this revision.May 18 2021, 7:35 AM

Thanks for the review! I've commit in 6381664580080f015bc0c2ec647853f697cf744a.

Looks like this breaks tests on macOS:
http://45.33.8.238/macm1/9739/step_7.txt
http://45.33.8.238/mac/31615/step_7.txt

Please take a look, and please revert for now if it takes a while to fix.

Looks like this breaks tests on macOS:
http://45.33.8.238/macm1/9739/step_7.txt
http://45.33.8.238/mac/31615/step_7.txt

Please take a look, and please revert for now if it takes a while to fix.

Thank you -- I think I've resolved the issue in ccbac06a072b86ea3b46479e478a1abee8520ef8

~Aaron