Skip to content

Conversation

@igchor
Copy link
Contributor

@igchor igchor commented May 14, 2024

Change unordered_map to vector as number of entries is expected to be low (1 in common case).

Also, do not store mapping between subdevices and the kernel handles. Instead, just look up kernel based on RootDevice handle for a sub device.

Additionally, use unique_ptr to store kernel_handle to avoid memory leaks in case zeKernelCreate fails.

@github-actions github-actions bot added the level-zero L0 adapter specific issues label May 14, 2024
@igchor igchor force-pushed the kernel_lookup_opt branch from 867233c to befa1a6 Compare May 14, 2024 22:52
@github-actions github-actions bot added the command-buffer Command Buffer feature addition/changes/specification label May 14, 2024
Change unordered_map to vector as number of entries is expected
to be low (1 in common case).

Also, do not store mapping between subdevices and the kernel handles.
Instead, just look up kernel based on RootDevice handle for a
sub device.

Additionally, use unique_ptr to store kernel_handle to avoid memory
leaks in case zeKernelCreate fails.
@igchor igchor force-pushed the kernel_lookup_opt branch from befa1a6 to 6835607 Compare May 17, 2024 00:10
@igchor
Copy link
Contributor Author

igchor commented May 18, 2024

After #1611 I started to look into how rest of the functions in kernel.cpp are implemented and I think some of them might not handle kernels created from native handles and regular ones correctly (when kernel is created for more than one device). For example, here,

ZE2UR_CALL(zeKernelSetIndirectAccess, (ZeKernel, IndirectFlags));
the properties are only set on the single ZeKernel while all others (from ZeKernelMap) are ignored. Similarly, the properties are only reported for that single ZeKernel:
ZE_CALL_NOCHECK(zeKernelGetProperties, (ZeKernel, &Properties));

@nrspruit @pbalcer Do you know if this is expected or should those functions actually set/query all ZeKernels from the ZeKernelMap?

Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

👍 the next step should be to get rid of zeKernel entirely :)

}

ze_kernel_handle_t getZeKernel(ur_device_handle_t Device) {
assert(Device);
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of devices is basically set the moment we initialize the driver. In my event cache rewrite, I took advantage of that to give monotonically increasing ID's to the devices, allowing us to make this type of operation a direct lookup in an array.
pbalcer@147f0b4#diff-89d38124dbcf1f91b31dc489a7e7c66bc0e6607ff77f2654810f74eae0826025R205

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 also thought about assigning ids to the devices on a context level. But you're right, it would be easier to do that globally, per-driver. We could do that in a separate PR probably.

@igchor igchor closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command-buffer Command Buffer feature addition/changes/specification level-zero L0 adapter specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants