[SYCL] vec abi changes behind -fpreview-breaking-changes flag

[cperkinsintel]

sycl::vec was updated earlier to be SYCL 2020 conformant. But those were ABI breaking changes, and the decision has been made to put those changes behind the -fpreview-breaking-changes / __INTEL_PREVIEW_BREAKING_CHANGES flag/macro.

This PR restores the old sycl::vec implementation and puts the new conforming one behind the flag/macro.

[bader]

This PR restores the old sycl::vec implementation and puts the new conforming one behind the flag/macro.

Technically, we added API versioning to handle "library-only" ABI changes like this. We should not need any flag/macro to change sycl::vec implementation.

[cperkinsintel]

ping to reviewers. I'd like to get this merged before the next pulldown

[AlexeySachkov]

TL;DR I would suggest not to allow users to select between vec ABI versions to avoid having to bugfix both. Instead, I would suggest to use versioning mechanisms to support old vec so that the runtime works with binaries build by older toolchains, but always use new vec for any application built with newer runtime.

Details:

Haven't looked into this in details, but here are my two cents which are more about high-level design/approach/direction for this particular change and future changes like this we may have:

I'm concerned about multi-versioning: my vec::convert change has landed first, but there are anyway some regressions reported form it - does it mean that I will have to apply changes twice for both "new" and "old" implementation?

This could be inevitable in general case of having different API/ABI between versions and I think it would be helpful to have some comments/documentation somewhere which would instruct a developer about which versions require update depending on the changes, which version is actual, which version is legacy, etc.

Note: in this particular case it seems to me that we can actually unify both versions more or less nicely, because at first glance they seem similar: there is a difference of how underlying data type is chosen, but when it comes to operators implementation there seem to be just two implementations which are enough to cover all 4 combinations we have (legacy with/without ext-vec-type, new with/without ext-vec-type).

I'm not asking to do that unification in scope of this PR, because it will require notable efforts and may not work out as good as I imagine. Plus, we could use this PR to troubleshoot our versioning mechanism we have:

This PR restores the old sycl::vec implementation and puts the new conforming one behind the flag/macro.
Technically, we added API versioning to handle "library-only" ABI changes like this. We should not need any flag/macro to change sycl::vec implementation.

This is a very good point and I would like to discuss it more. Do we have a use case where uses would actually need a control over vec ABI? Something like interoperability between object files compiled by different toolchain versions? Is it really common?

If we change our mindset to "new implementation is a current implementation, this is what we will have in new releases", then my previous question about "do I need to change both when I fix a bug" is a no question at all. "old" is only for backward compatibility, no new releases will be done with it and therefore no need to touch it at all. I only need to fix a new one for the upcoming release.

We would still have to maintain some #ifdef __breaking_changes macro because of API breaks we are not allowed to make which are required to do API bugfix or to match newer SYCL specs.

However, I doubt that this is a header-only ABI break, because there are host versions of math built-ins, which are implemented within the library and therefore we may have to duplicate all (?) those library entry points to maintain backward compatibility with older runtime. Will it cause even more code changes? Likely so. Will it increase maintenance burden more than having to vec implementations? I don't think so, because I expect vec interface or implementation to be changed/bugfixed more often that implementations of math built-ins for host.

[AlexeySachkov]

We had a discussion about this and we are not sure that we can be safe about passing SYCL-CTS with the old implementation. Considering other constraints we have, the decision was (if I understood it correctly) to proceed with this patch as-is as the only available option at the moment. Therefore, approving the PR (even though I'm not really a code owner for this code)

[AlexeySachkov]

I would vote for adding a comment here about how to proceed with bugfix/further changes to vec class. If I understand correctly, all new API/ABI breaking changes should only go into vec_new, whilst generic implementation bugfix should be done in both versions

[aelovikov-intel]

I'd very much prefer to have the split at the headers level, instead of having #ifs all the time. Maybe common part could be factored out to some helpers.

I see that it was a bad suggestion now... How hard would it be to return it back?

[aelovikov-intel]

LGTM given two issues with comments will be fixed before merge.