[SYCL] Add runtime support for dummy images for virtual functions #16087
Conversation
uditagarwal97
left a comment
uditagarwal97
left a comment
There was a problem hiding this comment.
@jzc Can you update the description of this PR with a summary and motivation behind these changes?
| // Byte array stores its size in first 8 bytes | ||
| size_t Shift = Prop->Type == SYCL_PROPERTY_TYPE_BYTE_ARRAY ? 8 : 0; | ||
| return ur::cast<const char *>(Prop->ValAddr) + Shift; | ||
| return {ur::cast<const char *>(Prop->ValAddr) + Shift, Prop->ValSize - Shift}; |
There was a problem hiding this comment.
Unrelated nitpicking, but this cast has nothing to do with ur and a standard C++ cast would fit better, I think.
| for (const sycl_device_binary_property &VFProp : | ||
| BinImage->getVirtualFunctions()) { | ||
| std::string StrValue = DeviceBinaryProperty(VFProp).asCString(); | ||
| if (VFProp->Name == std::string_view("dummy-image")) { |
There was a problem hiding this comment.
FYI: you can use sv string literal to make the code less verbose: cppreference
| // incompatible, then we should link its "dummy" version to avoid link | ||
| // errors about unresolved external symbols. | ||
| if (doesDevSupportDeviceRequirements(Dev, *BinImage)) | ||
| // Note: we only link when exactly one of |
| // already seen. | ||
| std::set<std::string> HandledSets; | ||
| std::queue<std::string> WorkList; | ||
| for (const sycl_device_binary_property &VFProp : Img.getVirtualFunctions()) { |
There was a problem hiding this comment.
Not asking to change it here, but in retrospect I think that the chosen name getVirtualFunctions is a bad one. The return value is a property, so the name should convey that.
| // just grab all sets they reference and add them for consideration if | ||
| // we haven't done so already. | ||
| bool isDummyImage = false; | ||
| for (const sycl_device_binary_property &VFProp : |
There was a problem hiding this comment.
So, we started with a device image which contains a kernel, obtained sets of virtual functions that it uses and now we iterate over device images from those sets, right?
We expect to see a device image with the dummy property, but your other change below to ProgramManager::addImages seems to make sure that no dummy images are ever added into set2binimage map. Am I missing something here, or there is a bug?
| generateImage({"KernelG"}, "set-f", /* uses vf set */ true, PROGRAM_F1), | ||
| generateImage({"KernelH"}, "set-h", /* uses vf set */ true, PROGRAM_H, | ||
| false, {}), | ||
| generateImage({"DummyKernel7"}, "set-h", /* provides vf set */ false, |
There was a problem hiding this comment.
Yeah, in retrospect, provides vf set false reads as "this does not provide a vf set", even though it does :)
It probably should have been a bool constant here instead of a comment, i.e.:
// Helpers for UsesVFSets argument of generateImage
constexpr bool provides_vf_set = false;
constexpr bool uses_vf_set = true;
// ...
generateImage(..., provides_vf_set, ...),
generateImage(..., uses_vf_set, ...),
// ...
Minor comment, we can (and probably should) address it in a separate PR to reduce amount of unrelated changes here
|
|
||
| CapturedLinkingData.clear(); | ||
|
|
||
| // KernelF uses set "set-h" that is also used by KernelG |
There was a problem hiding this comment.
| // KernelF uses set "set-h" that is also used by KernelG | |
| // KernelH uses set "set-h" that is also used by KernelG |
3 participants
No description provided.