[SYCL] ABI-Neutralize graph API

[bso-intel]

print_graph should use sycl::string_view instead of std::string which is not ABI-neutral.

[EwanC]

If I understand this right, the public facing API is still std::string, we're just changing the way it's implementated to use detail::string_view is that right? As graphs is an experimental status extension we can make ABI breaking changes outside of the ABI breaking windows, so don't need to use __INTEL_PREVIEW_BREAKING_CHANGES. That's what https://intel.github.io/llvm/developer/ABIPolicyGuide.html#changing-abi says in

Note: Features clearly marked as experimental are considered as an exception to this guideline.

[bso-intel]

@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?)

[bso-intel]

@stdale-intel
Could you clarify the exception rule that @EwanC mentioned above?

[steffenlarsen]

@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.

[gmlueck]

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?

[bso-intel]

@stdale-intel
Please confirm if we can break ABI or not.
Note that SYCL GRAPH is in the experimental list.
Thanks.

[stdale-intel]

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)

[EwanC]

Thanks for the feedback everyone

[bso-intel]

@intel/llvm-gatekeepers
Please merge

[aelovikov-intel]

Why is there no test change for this? We're supposed to have a test for the C++11 ABI-related things...

[againull]

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.