Skip to content

[SYCL] Fix issue with sub-sub-device in plugin #6349

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.

Sign up for GitHub

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

Merged
againull merged 3 commits into from
Jun 28, 2022
Merged

[SYCL] Fix issue with sub-sub-device in plugin #6349

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1edb69e
[SYCL] Fix issue with sub-sub-device in plugin
asudarsa Jun 23, 2022
0d3654a
Address review changes
asudarsa Jun 27, 2022
dce6a07
Add comment
asudarsa Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions sycl/plugins/level_zero/pi_level_zero.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,20 +684,26 @@ pi_result _pi_device::initialize(int SubSubDeviceOrdinal,
ZE_CALL(zeDeviceGetCommandQueueGroupProperties,
(ZeDevice, &numQueueGroups, QueueGroupProperties.data()));

// Initialize a sub-sub-device with its own ordinal and index
// Initialize ordinal and compute queue group properties
for (uint32_t i = 0; i < numQueueGroups; i++) {
if (QueueGroupProperties[i].flags &
ZE_COMMAND_QUEUE_GROUP_PROPERTY_FLAG_COMPUTE) {
QueueGroup[queue_group_info_t::Compute].ZeOrdinal = i;
QueueGroup[queue_group_info_t::Compute].ZeProperties =
QueueGroupProperties[i];
break;
}
}

// Reinitialize a sub-sub-device with its own ordinal, index and numQueues
// Our sub-sub-device representation is currently [Level-Zero sub-device
// handle + Level-Zero compute group/engine index]. As we have a single queue
// per device, we need to reinitialize numQueues in ZeProperties to be 1.
if (SubSubDeviceOrdinal >= 0) {
QueueGroup[queue_group_info_t::Compute].ZeOrdinal = SubSubDeviceOrdinal;
QueueGroup[queue_group_info_t::Compute].ZeIndex = SubSubDeviceIndex;
} else { // This is a root or a sub-device
for (uint32_t i = 0; i < numQueueGroups; i++) {
if (QueueGroupProperties[i].flags &
ZE_COMMAND_QUEUE_GROUP_PROPERTY_FLAG_COMPUTE) {
QueueGroup[queue_group_info_t::Compute].ZeOrdinal = i;
QueueGroup[queue_group_info_t::Compute].ZeProperties =
QueueGroupProperties[i];
break;
}
}
QueueGroup[queue_group_info_t::Compute].ZeProperties.numQueues = 1;
Copy link
Contributor

@againull againull Jun 24, 2022

Choose a reason for hiding this comment

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

Should we initialize all fields of ZeProperties (there are more of them, see https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=numqueues#ze-command-queue-group-properties-t)?

Can we reuse zeDeviceGetCommandQueueGroupProperties result from here https://github.com/intel/llvm/pull/6349/files#diff-15dd1eb076d2164bd9e87d9057f05f652a716498e8cdf5975e564c65309a0985R684-R685? If I am not mistaken ZeProperties is supposed to be initialized using zeDeviceGetCommandQueueGroupProperties and not manually.

Copy link
Contributor Author

@asudarsa asudarsa Jun 24, 2022

Choose a reason for hiding this comment

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

L0 backend does not have any subsub devices. In the current implementation, we piggy back on the sub-device to act as the ZeDevice for subsubdevice. So, the zeDeviceGetCommandQueueGroupProperties returns the properties for sub-device and not the subsub device. So, that's why I am not using it here.

Copy link
Contributor Author

@asudarsa asudarsa Jun 24, 2022

Choose a reason for hiding this comment

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

Also, about initializing all other fields. I think we can try to initialize minimal number of fields. Currently, we need only the number of queues.

Copy link
Contributor

@againull againull Jun 24, 2022

Choose a reason for hiding this comment

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

Sorry, but I have troubles understanding why it is ok to initialize only numQueues.

For example, at this part of code in enqueueMemFillHelper we use maxMemoryFillPatternSize:
https://github.com/intel/llvm/blob/sycl/sycl/plugins/level_zero/pi_level_zero.cpp#L6399-L6419

If Device at this part of code is a sub-sub device then we use uninitialized field maxMemoryFillPatternSize there.

Copy link
Contributor

@jandres742 jandres742 Jun 25, 2022

Choose a reason for hiding this comment

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

As @asudarsa mentions, L0 doesn't have sub-sub-devices, to the properties you can get are the ones from the sub-device. I think that we have two options:

  • whenever querying for props of a sub-sub-devices, query the ones from the parent sub-devices, either sub-device->props or a pointer to it.
  • copy them at initialization.

Copy link
Contributor

@smaslov-intel smaslov-intel Jun 27, 2022

Choose a reason for hiding this comment

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

I agree for sub-sub-device we need to initialize the entire ZeProperties (of type ze_command_queue_group_properties_t), populate it with what it's parent sub-device has in zeDeviceGetCommandQueueGroupProperties and then update as makes sense, e.g. set numQueues = 1

Copy link
Contributor Author

@asudarsa asudarsa Jun 27, 2022

Choose a reason for hiding this comment

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

Hi @smaslov-intel and @jandres742

Thanks so much for the feedback. As you mentioned, I think it is a good idea to initialize the entire ZeProperties structure using data gathered for sub-device. However, zeDeviceGetCommandQueueGroupProperties returns multiple ZeProperties, one for each group. I think I will try to use the one for 'compute' group. Please let me know if there are questions.

Thanks again

Copy link
Contributor Author

@asudarsa asudarsa Jun 27, 2022

Choose a reason for hiding this comment

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

Added a new commit addressing changes. Thanks

Copy link
Contributor

@smaslov-intel smaslov-intel Jun 27, 2022

Choose a reason for hiding this comment

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

Looks good, but please add a comment why "numQueues" is updated for sub-sub-device

} else { // Proceed with initialization for root and sub-device
// How is it possible that there are no "compute" capabilities?
if (QueueGroup[queue_group_info_t::Compute].ZeOrdinal < 0) {
return PI_ERROR_UNKNOWN;
Expand Down