Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCLomatic] Fix incorrect typecast of sycl::malloc return type in case of CUDA type redefination #2146

Conversation

the-slow-one
Copy link
Collaborator

@the-slow-one the-slow-one commented Jul 9, 2024

The following migrated code in DPCT have incompatiable cast operation. This isssue is only relavent on Windows. The sample below only compiles on Windows.

// issue.dp.cpp
#include <sycl/sycl.hpp>
#include <dpct/dpct.hpp>
#include <cstdint>

typedef uint64_t CUdeviceptr;

void foo(dpct::device_ptr ptr) {
  ptr = DPCT_CHECK_ERROR(
      DPCT_CHECK_ERROR(ptr = (unsigned long long)sycl::malloc_device(
                           1024, dpct::get_in_order_queue())));
}

Orginal CUDA code is

#include <cstdint>
#include <cuda.h>

typedef uint64_t CUdeviceptr;

void foo(CUdeviceptr ptr) {
  ptr = cuMemAlloc(&ptr, 1024);
}

Solution is to stop the underlying type lookup if the type is a defined by CUDA. (Existing implementation only looks if the source of defination is cuda.h but it's not sufficent as evident in the CUDA example above)

@the-slow-one the-slow-one requested a review from a team as a code owner July 9, 2024 17:49
Copy link
Contributor

@AndyCHHuang AndyCHHuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation LGTM.
Please add a lit test.

@the-slow-one the-slow-one requested review from AndyCHHuang and removed request for tomflinda August 29, 2024 13:00
@zhiweij1
Copy link
Contributor

@the-slow-one
The test case has syntax error, it cannot be compiled by nvcc:

zhiwei@zhiwei-ubuntu:~/testfolder$ nvcc test.cu -c
test.cu(4): error: invalid redeclaration of type name "CUdeviceptr" (declared at line 262 of /usr/local/cuda-12.4/bin/../targets/x86_64-linux/include/cuda.h)
  typedef uint64_t CUdeviceptr;
                   ^

test.cu(7): error: argument of type "CUdeviceptr *" is incompatible with parameter of type "CUdeviceptr *"
    ptr = cuMemAlloc_v2(&ptr, 1024);
                        ^

2 errors detected in the compilation of "test.cu".
zhiwei@zhiwei-ubuntu:~/testfolder$ 

@the-slow-one
Copy link
Collaborator Author

@the-slow-one The test case has syntax error, it cannot be compiled by nvcc:

zhiwei@zhiwei-ubuntu:~/testfolder$ nvcc test.cu -c
test.cu(4): error: invalid redeclaration of type name "CUdeviceptr" (declared at line 262 of /usr/local/cuda-12.4/bin/../targets/x86_64-linux/include/cuda.h)
  typedef uint64_t CUdeviceptr;
                   ^

test.cu(7): error: argument of type "CUdeviceptr *" is incompatible with parameter of type "CUdeviceptr *"
    ptr = cuMemAlloc_v2(&ptr, 1024);
                        ^

2 errors detected in the compilation of "test.cu".
zhiwei@zhiwei-ubuntu:~/testfolder$ 

This is a Windows-only test. nvcc compiles this file on Windows.

Co-authored-by: Jiang, Zhiwei <zhiwei.jiang@intel.com>
@zhimingwang36 zhimingwang36 merged commit cbf7309 into oneapi-src:SYCLomatic Sep 2, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants