[SYCL][ABI-Break] Re-design the handling of optional code location argument for USM calls and honor memory allocation properties

[smaslov-intel]

Do not drop properties specified by user.

Signed-off-by: Sergey V Maslov [email protected]

[bader]

This fixes just one function out of eight. The properties are dropped by malloc, malloc_device, malloc_host, malloc_shared, aligned_alloc_device, aligned_alloc_host, aligned_alloc_shared. Shouldn't we fix all of them?

[smaslov-intel]

This fixes just one function out of eight. The properties are dropped by malloc, malloc_device, malloc_host, malloc_shared, aligned_alloc_device, aligned_alloc_host, aligned_alloc_shared. Shouldn't we fix all of them?

We probably should. I fixed where I saw the immediate problem reported. Do you prefer we do all at once in this PR?

[smaslov-intel]

BTW, my change broke ABI test (due to new argument added).

c++filt
The following symbols are missing from the new object file:
_ZN2cl4sycl13aligned_allocEmmRKNS0_6deviceERKNS0_7contextENS0_3usm5allocENS0_6detail13code_locationE

I guess I should just leave the existing version and add one that accepts properties, right?
Or, maybe we are allowed to break ABI now?

[aelovikov-intel]

Or, maybe we are allowed to break ABI now?

We are allowed.

[bader]

This fixes just one function out of eight. The properties are dropped by malloc, malloc_device, malloc_host, malloc_shared, aligned_alloc_device, aligned_alloc_host, aligned_alloc_shared. Shouldn't we fix all of them?

We probably should. I fixed where I saw the immediate problem reported. Do you prefer we do all at once in this PR?

Yes. It's better to fix all them at once and don't wait 7 other bug reports from customers to fix. :)

[smaslov-intel]

It's better to fix all them at once

@bader, sure, it would just cause this customer problem to hang for some more time.
I now find quite a few duplicates of the alloc routines seeming to do with an extra argument, which is not in the spec:

const detail::code_location CL

Any idea where are these coming from and which should be removed, if any?

[smaslov-intel]

It's better to fix all them at once

@bader, sure, it would just cause this customer problem to hang for some more time. I now find quite a few duplicates of the alloc routines seeming to do with an extra argument, which is not in the spec:

const detail::code_location CL

Any idea where are these coming from and which should be removed, if any?

It looks they are coming from #5391
But why do we have version with "code_location" and without it?
Which one should stay? (SYCL spec doesn't know about code_location, which is from "detail")

[bader]

I don't know for sure, but I speculate that it's added to support the tools added with #5389 and sycl-sanitize specifically.
I hope someone from @intel/llvm-reviewers-runtime team and provide more details about that. Unfortunately, all folks working on this are not available.

[smaslov-intel]

I don't know for sure, but I speculate that it's added to support the tools added with #5389 and sycl-sanitize specifically. I hope someone from @intel/llvm-reviewers-runtime team and provide more details about that. Unfortunately, all folks working on this are not available.

Thanks! I am specifically worried that we are polluting the "sycl" namespace with something that is not a part of the SYCL spec.

[bader]

I don't know for sure, but I speculate that it's added to support the tools added with #5389 and sycl-sanitize specifically. I hope someone from @intel/llvm-reviewers-runtime team and provide more details about that. Unfortunately, all folks working on this are not available.

Thanks! I am specifically worried that we are polluting the "sycl" namespace with something that is not a part of the SYCL spec.

I remember that this approach was originally added by @tovinkere to enable tools support. #1129

I vaguely recall discussing this concern with him and he convinced me that there should be no impact on users.

[smaslov-intel]

I remember that this approach was originally added by @tovinkere to enable tools support. #1129

Thanks @bader ! I discussed this with @tovinkere and uploaded a patch that I think does what is wanted.
It follows #1129 and allows to disable the optional last code_location argument with DISABLE_SYCL_INSTRUMENTATION_METADATA.

[bader]

Please, update declarations in the header file - sycl/include/sycl/usm.hpp.

[smaslov-intel]

Please, update declarations in the header file - sycl/include/sycl/usm.hpp.

Done

[smaslov-intel]

The fails in SYCL / Linux / ESIMD Emu LLVM Test Suite (pull_request) are not related to this PR:

Failed Tests (3):
SYCL :: dpas/dpas_test1.cpp
SYCL :: dpas/dpas_test2.cpp
SYCL :: dpas/dpas_test3.cpp

[bader]

👍 Thanks!

[tovinkere]

Moving _CODELOCPARAM and other macros to common.hpp and simplifying the default argument and its use in the implementation will make it more readable and define a template for use in other places in the future.

[smaslov-intel]

The ESIMD and CUDA failures are unrelated.

[steffenlarsen]

Looks like it breaks ABI so the title should reflect that and the ABI version should be incremented.

Also, before it gets merged it may be worth checking the windows dump and making the corresponding changes there as well so we avoid post-commit failures.

[smaslov-intel]

Also, before it gets merged it may be worth checking the windows dump and making the corresponding changes there as well so we avoid post-commit failures.

@steffenlarsen : I am getting strange errors when trying to configure the build:

$ python buildbot/configure.py -t Debug
...
-- Looking for _M_X64
CMake Error: Unable to open check cache file for write.

Is there a way to trigger Win build in CI/pre-commit?

[steffenlarsen]

LGTM! 😄

[smaslov-intel]

Anything prevents this from being merged?

[steffenlarsen]

Anything prevents this from being merged?

I don't think so. Merging!