llvm
llvm copied to clipboard
intel
[SYCL] ABI-Neutralize graph API
print_graph should use sycl::string_view instead of std::string which is not ABI-neutral.
@steffenlarsen I think you wrote the exception sentence to the ABI changing rule. Could you confirm if what @EwanC said is true? (ie, can I make ABI-breaking changes even outside the window?)
@stdale-intel Could you clarify the exception rule that @EwanC mentioned above?
@steffenlarsen I think you wrote the exception sentence to the ABI changing rule. Could you confirm if what @EwanC said is true? (ie, can I make ABI-breaking changes even outside the window?)
Ewan is right, experimental APIs are exempt from the ABI-break rules... That said, graph is one of the more popular extensions, so I believe we need to be a little careful still.
Tag @stdale-intel & @gmlueck & @jbrodman . Note: this would only affect user-code that contains calls to print_graph.
Ewan is right, experimental APIs are exempt from the ABI-break rules... That said, graph is one of the more popular extensions, so I believe we need to be a little careful still.
Tag @stdale-intel & @gmlueck & @jbrodman . Note: this would only affect user-code that contains calls to
print_graph.
In my opinion it's OK to break ABI, but we should notify internal groups that we know are using this feature. Other opinions @stdale-intel?
@stdale-intel Please confirm if we can break ABI or not. Note that SYCL GRAPH is in the experimental list. Thanks.
While @EwanC is technically correct here, I specifically asked @bso-intel to add these anways. We have made a major push on getting folks to use graph items with the recently oneapi release. We should go out of our way to not break those folks.
In this specific case here, this change is most urgently being requested by teams already using the preview-breaking-changes and so it not impactful to them. while the majority of users will not care/even be aware of this implementation level detail so breaking them right after a major release would not be a good experience versus guarding this change which costs us nothing (as least as far as I understand it)
Why is there no test change for this? We're supposed to have a test for the C++11 ABI-related things...
Why is there no test change for this? We're supposed to have a test for the C++11 ABI-related things...
We have a test https://github.com/intel/llvm/blob/sycl/sycl/test/abi/sycl_abi_neutrality_test.cpp#L25 which covers this symbol but that test checks only libsycl.so. Changes in this PR are done under __INTEL_PREVIEW_BREAKING_CHANGES. We don't have analogue of sycl_abi_neutrality_test.cpp for libsycl_preview.so, I might had wrong understanding that we don't need it. I will cover libsycl_preview.so there too then.
