[SYCL][Docs] Mention preview macro for early ABI breaks #18422
Conversation
This commit makes a mention of ABI breaking changes being made under preview macro guards to help ensuring they get promoted during the ABI breaking window. Signed-off-by: Larsen, Steffen <[email protected]>
| symbols) can be done under the `__INTEL_PREVIEW_BREAKING_CHANGES` macro. This | ||
| helps maintainers make sure that the ABI-breaking changes makes it in during the | ||
| ABI-breaking window, as they will be considered for promotion out of the | ||
| `__INTEL_PREVIEW_BREAKING_CHANGES` guards during that time. |
There was a problem hiding this comment.
We have inline namespace with API/ABI version. Why do we ignore this framework for making breaking changes and use a macro instead?
There was a problem hiding this comment.
I don't think they are necessarily mutually exclusive, though historically we have used the preview macro more. I personally prefer the breaking changes macro, as it makes it easier to make it clear what is going away and what is coming in with the breaking changes. It also lets us test that we can indeed remove the symbols that we intend on removing, rather than finding out during the limited ABI-break window that we still need to remove some uses of old symbols and/or someone added more without realizing.
There was a problem hiding this comment.
I highly recommend using symbol versioning. It enables "preview" features in non-breaking manner.
If SYCL runtime team against using versioning for some reasons, please, remove it from the library sources with the next API/ABI breaking release.
I don't think they are necessarily mutually exclusive, though historically we have used the preview macro more.
Symbol versioning has never been used.
I personally prefer the breaking changes macro, as it makes it easier to make it clear what is going away and what is coming in with the breaking changes.
I'm not sure I understand what makes macro to be clearer than a separate namespace.
The main advantage of preview macro solution is small code to compile. As adding a new namespace is non-breaking change, old symbols versions must be compiled as well.
There was a problem hiding this comment.
If SYCL runtime team against using versioning for some reasons, please, remove it from the library sources with the next API/ABI breaking release.
I don't think they are necessarily mutually exclusive, though historically we have used the preview macro more.
Symbol versioning has never been used.
I am personally ambivalent towards it, but every time we've wanted to use it, it was insufficient or brought on other issues. It has been a while though, so I don't remember the details.
I'm not sure I understand what makes macro to be clearer than a separate namespace.
When using the preview macro, it is much easier to place the breaks close to the old changes. For example, if you want to make an API break inside a function, you can just make the changes inside the function, while with the versioning you have to place a copy of the function (or split out the functionality) into a different namespace somewhere potentially far from the current function.
For ABI breaking changes, the preview macro guard has two benefits:
- You can place the changes close to (or even inside) the old symbols.
- You can mask out the old function, ensuring that we can test the library without it. I mentioned that in the previous comment, but it helps us avoid using the symbols we intend to remove and also makes it clear which ones they are.
There was a problem hiding this comment.
@vinser52, please, be aware of this discussion.
@steffenlarsen, just in case you don't follow the SYCL runtime upstreaming progress.
@KseniyaTikhomirova is pushing library API versioning to upstream. See llvm/llvm-project#144372.
There was a problem hiding this comment.
What about cases where we want to make ABI break changes early? If the point is to use versioning to avoid conflicts when updating symbols, I don't think there's any conflict with the preview flag.
I agree that versioning via namespaces and __INTEL_PREVIEW_BREAKING_CHANGES macro are orthogonal. My point is that if it is possible to introduce a new API using the new version namespace, we should do it instead of using the __INTEL_PREVIEW_BREAKING_CHANGES macro.
Fair point, it was too quick of an example. My point was, what happens in a case where you have to change the API once again? Does it go into v3?
The question is whether we want to support backward compatibility or not. And my point is how often such cases happen. But if it happens, for sure we need the new version, but then the question is how costly it is. For example, if we introduce the new version of class A and there are other classes that use it we might need to change them as well.
There was a problem hiding this comment.
I suppose class A is actually sycl::detail::A (because otherwise the signature is dictated by the spec and we simply can't change it). Nothing prevents us from adding A2 and using it in place of A in the headers, while still providing old A exports/interfaces.
There was a problem hiding this comment.
I suppose
class Ais actuallysycl::detail::A(because otherwise the signature is dictated by the spec and we simply can't change it).
For sure, it can be used for the internal types.
Regarding types defined by the SYCL spec it is an interesting question. If we use inline namespace (for example sycl::v1::A and v1 is an inline namespace) user code can still use sycl::A. I am not sure if it contradicts the SYCL spec.
Nothing prevents us from adding
A2and using it in place ofAin the headers, while still providing oldAexports/interfaces.
I think it is less flexible because with inline namespaces we can rely on the compiler to decide which version should be used by using just sycl::A and the compiler will choose the version from the inline namespace.
There was a problem hiding this comment.
I am not sure if it contradicts the SYCL spec.
When we added versioning we assumed it's not, because source complies to the standard and other representations (e.g. AST, mangled symbols, LLVM IR) are regulated by the implementation.
There was a problem hiding this comment.
BTW, we are already using inline namespace _V1 for versioning of public API. For example sycl::handler is in fact sycl::_V1::handler
This commit makes a mention of ABI breaking changes being made under preview macro guards to help ensuring they get promoted during the ABI breaking window.