For NPU Sample, add flexibility in device creation options and fix Generic ML Device logic #624
Conversation
…neric ML Device logic Expand NPU sample's capabilities for creating devices based on attributes. More specifically, allow options to filter based on allowed, unallowed, and required attributes. Then add some flags for the most commonly needed options for this sample. The logic for how each interacts with CreateAdapterList is a bit unconventional since CreateAdapterList ANDs the passed in attributes. Tested locally on Intel NPU.
…atch For NPU Sample, add flexibility in device creation options and fix Generic ML Device logic
| { | ||
| size_t adapterNameSize = 0; | ||
| THROW_IF_FAILED(adapter->GetPropertySize(DXCoreAdapterProperty::DriverDescription, &adapterNameSize)); | ||
| char* adapterName = (char*)malloc(adapterNameSize); |
There was a problem hiding this comment.
Where is the memory released?
Better to just use std::string and almost never touch malloc.
There was a problem hiding this comment.
DirectML/DxDispatch/src/dxdispatch/Adapter.h
Lines 32 to 51 in a87891e
There was a problem hiding this comment.
I think this sample should be kept as simple as possible and only show logic to unambiguously select an NPU. I don't think we need to illustrate the flexibility and numerous ways adapters can be filtered (this isn't a DXCore sample); I think this flexibility actually makes the sample more confusing, because we're not guiding users on the precise attributes required to leverage NPU.
In short, I suggest having something like GENERIC_ML || (CORE_COMPUTE && !GRAPHICS).
| // filter out adapters with disallowed attributes | ||
| for (auto& disallowedGuid : dxGuidDisallowedAttributes) | ||
| { | ||
| if (currentGpuAdapter->IsAttributeSupported(disallowedGuid)) { isAdapterValid = false; } |
There was a problem hiding this comment.
Is there any reason to continue checking all the remaining attributes if any of them is false? One can early-out and just break.
for (auto& disallowedGuid : dxGuidDisallowedAttributes)
{
if (currentGpuAdapter->IsAttributeSupported(disallowedGuid))
{
isAdapterValid = false;
break;
}
}
There was a problem hiding this comment.
Btw, if using C++20 (I'm unsure the minimum C++ version used in this sample), you could use:
isAdapterValid = std::ranges::all_of(dxGuidRequiredAttributes, [&](const& guid) {return currentGpuAdapter->IsAttributeSupported(guid)})
isAdapterValid &= std::ranges::all_of(dxGuidDisallowedAttributes, [&](const& guid) {return !currentGpuAdapter->IsAttributeSupported(guid)})
| THROW_IF_FAILED(factory->CreateAdapterList(ARRAYSIZE(dxGUIDs), dxGUIDs, IID_PPV_ARGS(&adapterList))); | ||
| for (uint32_t i = 0, adapterCount = adapterList->GetAdapterCount(); i < adapterCount; i++) | ||
| // If there's any required attributes, save time by only passing in required attributes instead. | ||
| std::vector<GUID> iteratingAdapterList = dxGuidRequireAllAttributes.empty() ? dxGuidAllowedAttributes : dxGuidRequireAllAttributes; |
There was a problem hiding this comment.
| std::vector<GUID> iteratingAdapterList = dxGuidRequireAllAttributes.empty() ? dxGuidAllowedAttributes : dxGuidRequireAllAttributes; | |
| const std::vector<GUID>& iteratingAdapterList = dxGuidRequireAllAttributes.empty() ? dxGuidAllowedAttributes : dxGuidRequireAllAttributes; |
No need to make a copy of it, since it's not being mutated.
OR if using C++20, std::span.
|
Can this run on NPU of Snapdragon X series as well? |
Expand NPU sample's capabilities for creating devices based on attributes.
More specifically, allow options to filter based on allowed, unallowed, and required attributes. Then add some flags for the most commonly needed options for this sample.
The logic for how each interacts with CreateAdapterList is a bit unconventional since CreateAdapterList ANDs the passed in attributes.
Tested locally on Intel NPU.