[SYCL] Add the notion of a "default" -cc1 SYCL mode. #3702
Conversation
This defaults to SYCL 2020 currently, but makes it easier to change to a new default later. This default kicks in only in -cc1 mode when the user specifies -fsycl-is-device or -fsycl-is-host but does not pass -sycl-std= to the invocation. This does not impact the driver behavior.
AaronBallman
requested review from
elizabethandrews and
premanandrao
as code owners
There was a problem hiding this comment.
I'm generally ok here, I'm a touch worried how much 'dead' code got removed though :)
That said, I think the tests changed need to either do exactly what the actual sycl headers do (I think it is supposed to be const-ref by standard? Anyone know?), or just use our shim-headers.
If you have an example of what you'd like me to switch to, I can, but I can't find anything in the SYCL spec that says passing by value is deprecated in 2020 when I search for the word "deprecated" in the spec. |
I don't think they actually 'deprecated' so much as 'replaced' all the uses in the standard... They basically broke the API. But if you go here: https://www.khronos.org/registry/SYCL/specs/sycl-2020/pdf/sycl-2020.pdf around page 285 it looks like kernel-lambdas/functors are passed as const-ref. |
Ah, thanks! I'll make the change to the tests, but... I'm surprised we're not using universal references there. |
Maybe in the next version :) I agree, universal references make more sense. That said, you might consider if these are possible to do so, switching these tests over to the Inputs/sycl.hpp stuff that @srividya-sundaram worked on for a while. |
Yes, tests are not following new FE test guideline. This can be done in a separate PR. |
I opted to go the quick route and use const ref -- I like the idea of doing a separate PR to switch to the mock headers (though, we may still want to be careful to test things the mock headers don't do just to be sure we give sensible diagnostics on unexpected code). |
Yep, sounds good to me. I thought it might be an equal/only slightly more difficult task to switch these over 'while you are here'. You'd mentioned that these are pretty intensive changes, so I think we can leave these with just the |
|
The build failures from Jenkins appear to be unrelated (they're complaining about an unknown |
|
I think this patch can go to llorg as major part of the original code is already upstremed. |
A major part is upstreamed, but a critical piece is not (and I'm not certain we're ready to): there's no SYCL 2020 mode upstream, so setting the default mode would either require me to introduce SYCL 2020 (could be contentious if there's no actual functionality to attach to it, but we'd want it to be the default), or to have different default modes between upstream and DPC++. So perhaps this one should wait, WDYT @bader? |
|
I agree that it would be reasonable to upstream SYCL 2020 support for |
|
I filed https://reviews.llvm.org/D102261, we'll see where it goes in community. |
This defaults to SYCL 2020 currently, but makes it easier to change to
a new default later. This default kicks in only in -cc1 mode when the
user specifies -fsycl-is-device or -fsycl-is-host but does not pass
-sycl-std= to the invocation. This does not impact the driver behavior.