[SYCL] remove alignedAllocHost's Kind param

[ldrumm]

alignedAllocHost was introduced as part of what was described as a bugfix in 01869a0 wherin alignedAlloc was simply renamed to alignedAllocHost, but the Kind parameter (host|device|shared) was left unchanged, and the bugfix actually appears to be in the calling code. I'm not quite sure what happened there, but in any case we shouldn't have a Kind parameter if we only ever intend to work on the host (indeed the existing code treats Kind == host as an invariant, returning an error otherwise).

It looks like a couple of other people tried to work around this when adding other features (e.g. in 7df3923) but didn't fix this due to perceived ABI breaks. However, this function hasn't been exposed as part of the abi due to a declaration / definition mismatch between the header and implementation, so it was never callable in the first place.

Thus, this is a putatively NFC cleanup, but it's a change to the SYCL runtime ABI so not "genuine-NFC".

[maarquitos14]

I'm confused. If this is breaking ABI, why is there no change to sycl/test/abi/*?

[ldrumm]

I'm confused. If this is breaking ABI, why is there no change to sycl/test/abi/*?

Doh! because I forgot.

Will add

[maarquitos14]

BTW, we're not in ABI-breaking window, so this will have to wait until the next one.

[ldrumm]

I'm confused. If this is breaking ABI, why is there no change to sycl/test/abi/*?

Doh! because I forgot.

Will add

Just tried to do this and noticed that this symbol was never in the dump in the first place. After some head scratching, It looks like the declaration in the header never had a matching definition so no symbol was emitted.

This is because of function overloading. The declaration in the header was

__SYCL_EXPORT void *alignedAllocHost(size_t Alignment, size_t Bytes,
                                     const context &Ctxt, sycl::usm::alloc Kind,
                                     const code_location &CL);

The definition in usm_impl.cpp was

void *alignedAllocHost(size_t Alignment, size_t Size, const context &Ctxt,
                       alloc Kind, const property_list &PropList,
                       const detail::code_location &CodeLoc) {

Notice that the signatures don't match.

Since the definition in the cpp file didn't get the __SYCL_EXPORT attributes applied, the symbol never got made public in the DSO.

Thus, this is not actually ABI-breaking since it was never public in the first place and no changes to the dump file need be made (on GNU toolchains, at least; need to check win32)

[ldrumm]

If my analysis above is correct, would it make sense to just not expose this in the headers at all and move it into an anonymous namespace inside usm_impl.cpp?

[maarquitos14]

I'm confused. If this is breaking ABI, why is there no change to sycl/test/abi/*?

Doh! because I forgot.
Will add

Just tried to do this and noticed that this symbol was never in the dump in the first place. After some head scratching, It looks like the declaration in the header never had a matching definition so no symbol was emitted.

This is because of function overloading. The declaration in the header was

__SYCL_EXPORT void *alignedAllocHost(size_t Alignment, size_t Bytes,
                                     const context &Ctxt, sycl::usm::alloc Kind,
                                     const code_location &CL);

The definition in usm_impl.cpp was

void *alignedAllocHost(size_t Alignment, size_t Size, const context &Ctxt,
                       alloc Kind, const property_list &PropList,
                       const detail::code_location &CodeLoc) {

Notice that the signatures don't match.

Since the definition in the cpp file didn't get the __SYCL_EXPORT attributes applied, the symbol never got made public in the DSO.

Thus, this is not actually ABI-breaking since it was never public in the first place and no changes to the dump file need be made (on GNU toolchains, at least; need to check win32)

That explains why CI passed. If it was indeed an ABI break, and no changes in the related tests, CI should have failed.

[maarquitos14]

If my analysis above is correct, would it make sense to just not expose this in the headers at all and move it into an anonymous namespace inside usm_impl.cpp?

@AlexeySachkov what's your opinion on this? IMO, if this was never exposed, it sounds good to me.

[AlexeySachkov]

If my analysis above is correct, would it make sense to just not expose this in the headers at all and move it into an anonymous namespace inside usm_impl.cpp?

@AlexeySachkov what's your opinion on this? IMO, if this was never exposed, it sounds good to me.

If it was never exposed and used from public headers (only declared there), then for sure it should be moved down to the library. Absolutely no objections to that

[ldrumm]

It's now in an anonymous namespace, and I've verified win32 as well. Thus I will remove the ABI-BREAK tag, and I think this is ready to merge (pending CI)

[maarquitos14]

LGTM.