Skip to content

Conversation

@mbrookhart
Copy link
Contributor

@masahi
Copy link
Member

masahi commented Mar 16, 2021

Since this is not a opencl specific problem, ideally we should think about how to handle this case in all backends. For now I think we should have the same change for cuda, rocm, and vk as well.

@mbrookhart
Copy link
Contributor Author

I don't think cuda has this issue, I this cudamalloc does this under the hood, as does standard C malloc. I agree, though, it looks like we need it for rocm and vulkan.

@tmoreau89
Copy link
Contributor

Can the runtime attempt to free a buffer that was allocated that is of size 0? Could that trigger a segfault?

tmoreau89 pushed a commit to tmoreau89/tvm that referenced this pull request Mar 16, 2021
tmoreau89 pushed a commit to tmoreau89/tvm that referenced this pull request Mar 16, 2021
@mbrookhart
Copy link
Contributor Author

Feasibly. I didn't see such an issue with opencl, but indeed memory freeing doesn't pass in size, so I can't do the same check:

void OpenCLWorkspace::FreeDataSpace(TVMContext ctx, void* ptr) {
// We have to make sure that the memory object is not in the command queue
// for some OpenCL platforms.
OPENCL_CALL(clFinish(this->GetQueue(ctx)));
cl_mem mptr = static_cast<cl_mem>(ptr);
OPENCL_CALL(clReleaseMemObject(mptr));
}

@mbrookhart
Copy link
Contributor Author

But it would be easy to check for null before freeing

@masahi
Copy link
Member

masahi commented Mar 16, 2021

Maybe we can implement all book keeping stuff in DeviceAPI base class? After we make sure size is valid, we forward calls to derived class.

@masahi masahi self-assigned this Mar 16, 2021
@mbrookhart
Copy link
Contributor Author

It's messy, but we could write a default method that checks for these things and then calls a helper function. We then mark the default function as final and change ALL of the device classes to override the helper functions.

tmoreau89 pushed a commit to tmoreau89/tvm that referenced this pull request Mar 17, 2021
@mbrookhart
Copy link
Contributor Author

Closing in favor of #7691

@mbrookhart mbrookhart closed this Mar 18, 2021
@mbrookhart mbrookhart deleted the opencl_return_null_for_0_allocation branch March 18, 2021 20:32
tmoreau89 pushed a commit to tmoreau89/tvm that referenced this pull request Mar 19, 2021
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.

3 participants