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

Add UR_KERNEL_INFO_SPILL_MEM_SIZE kernel info prop #2614

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kurapov-peter
Copy link
Contributor

This allows for querying the information about produced register spills during kernel compilation. The query is optional and is only implemented for L0 now. The direct counterpart for the query is spillMemSize property returned by zeKernelGetProperties.

@kurapov-peter kurapov-peter requested review from a team as code owners January 24, 2025 11:33
@@ -299,6 +299,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelGetInfo(ur_kernel_handle_t hKernel,
hKernel->get()));
return ReturnValue(static_cast<uint32_t>(NumRegs));
}
case UR_KERNEL_INFO_SPILL_MEM_SIZE:
Copy link
Contributor

Choose a reason for hiding this comment

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

for here and in the equivalent file in the hip adapter you'll need to explicitly add a return UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION; - right now it's falling through to return UR_RESULT_ERROR_INVALID_ENUMERATION; which isn't the expected return for an unsupported optional query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

This should also update source/adapters/native_cpu/kernel.cpp.

@@ -299,6 +299,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelGetInfo(ur_kernel_handle_t hKernel,
hKernel->get()));
return ReturnValue(static_cast<uint32_t>(NumRegs));
}
case UR_KERNEL_INFO_SPILL_MEM_SIZE:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION rather than UR_RESULT_ERROR_INVALID_ENUMERATION.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -234,6 +234,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelGetInfo(ur_kernel_handle_t hKernel,
hKernel->get()));
return ReturnValue(static_cast<uint32_t>(NumRegs));
}
case UR_KERNEL_INFO_SPILL_MEM_SIZE:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return UR_RESULT_ERROR_UNSUPPORTED_ENUMERATION rather than UR_RESULT_ERROR_INVALID_ENUMERATION.

@kurapov-peter
Copy link
Contributor Author

This should also update source/adapters/native_cpu/kernel.cpp.

I see that in the native cpu adapter some other properties are also missing, e.g., UR_KERNEL_INFO_NUM_REGS. Should I add that too?

@kbenzie
Copy link
Contributor

kbenzie commented Jan 24, 2025

This should also update source/adapters/native_cpu/kernel.cpp.

I see that in the native cpu adapter some other properties are also missing, e.g., UR_KERNEL_INFO_NUM_REGS. Should I add that too?

Just this one should be fine, it's WIP.

@kurapov-peter
Copy link
Contributor Author

@pbalcer, what would be a proper way of implementing the v2 portion? The API now doesn't include the device handle which is needed to get the property through ur_kernel_handle_t_::getProperties(ur_device_handle_t hDevice). Returning a map for each device also doesn't seem like an option, as it breaks the interface.

@kurapov-peter kurapov-peter requested a review from a team as a code owner January 24, 2025 17:01
@@ -624,6 +624,16 @@ ur_result_t urKernelGetInfo(ur_kernel_handle_t hKernel,
case UR_KERNEL_INFO_NUM_REGS:
case UR_KERNEL_INFO_NUM_ARGS:
return ReturnValue(uint32_t{hKernel->getCommonProperties().numKernelArgs});
case UR_KERNEL_INFO_SPILL_MEM_SIZE: {
std::shared_lock<ur_shared_mutex> Guard(hKernel->getProgramHandle()->Mutex);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this from another similar implementation, assuming this protects the device retrieval call. Not sure if necessary, though.

Comment on lines +630 to +635
std::vector<uint32_t> spills(devices.size());
for (size_t i = 0; i < spills.size(); ++i) {
spills[i] = uint32_t{hKernel->getProperties(devices[i]).spillMemSize};
}
return ReturnValue(static_cast<const uint32_t *>(spills.data()),
spills.size());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This relies on the ReturnValue to preserve the memory for the vector.

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