[Target] Framework for device querying for all targets. #8602
Conversation
Lunderberg
requested review from
areusch,
junrushao,
kazum,
kparzysz-quic,
liangfu,
masahi,
tmoreau89,
tqchen,
vinx13 and
ZihengJiang
as code owners
Lunderberg
force-pushed
the
device_target_query
branch
from
ceebefe
to
5817143
Compare
|
@zxybazh , as this impacts the @tkonolige This was partly a follow-up from our conversation a few weeks ago, in which you mentioned that it would be nice to have device parameters query-able for cuda as well as vulkan. |
- Move "from_device" argument definition from "vulkan" target to all targets. - Add device querying to TargetInternal::FromConfig, using "from_device" argument. If present, these have lower priority than explicitly-specified attributes, but higher priority than the default attribute values. - Add default no-op DeviceAPI::GetTargetProperty.
Lunderberg
force-pushed
the
device_target_query
branch
from
5817143
to
34e7767
Compare
| @@ -106,7 +106,7 @@ class VulkanDeviceAPI final : public DeviceAPI { | |||
| * Returns the results of feature/property queries done during the | |||
| * device initialization. | |||
| */ | |||
| void GetTargetProperty(Device dev, const std::string& property, TVMRetValue* rv); | |||
| void GetTargetProperty(Device dev, const std::string& property, TVMRetValue* rv) final; | |||
There was a problem hiding this comment.
A bit curious about why adding final here?
There was a problem hiding this comment.
It isn't strictly necessary, but I try to be in the habit of including it for virtual functions that aren't intended to be subclassed further. It's always safe to remove final later, but until then it serves as a warning to my future self that I should be look into the class's implementation in detail before making any subclass of it.
I can go either way on having it or not, since final could be interpreted either as "not intended to be subclassed, so take a careful look first" or as "intended not to be subclassed, please don't change it". I tend to take the first interpretation, but the second is completely valid.
|
On target related class side, the code looks good to me. |
- Move "from_device" argument definition from "vulkan" target to all targets. - Add device querying to TargetInternal::FromConfig, using "from_device" argument. If present, these have lower priority than explicitly-specified attributes, but higher priority than the default attribute values. - Add default no-op DeviceAPI::GetTargetProperty. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
- Move "from_device" argument definition from "vulkan" target to all targets. - Add device querying to TargetInternal::FromConfig, using "from_device" argument. If present, these have lower priority than explicitly-specified attributes, but higher priority than the default attribute values. - Add default no-op DeviceAPI::GetTargetProperty. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
This has been a useful feature on the vulkan side, to determine device parameters based on the device present. This PR doesn't implement device querying on other targets, but moves the framework from vulkan-specific code to code that can be used by any runtime.
Move "from_device" argument definition from "vulkan" target to all targets.
Add device querying to TargetInternal::FromConfig, using "from_device" argument. If present, these have lower priority than explicitly-specified attributes, but higher priority than the default attribute values.
Add default no-op DeviceAPI::GetTargetProperty.