[Target] Add target_device_type attribute to override default device_type #12509

kparzysz-quic · 2022-08-20T00:02:58Z

Implement Target::GetTargetDeviceType (C++) or get_target_device_type (python) to get the device type (kDL...) for a given target.

The attribute "target_device_type" can be used to override the default device type associated with the target kind.

kparzysz-quic · 2022-08-23T12:00:40Z

I tried to start a discussion on the forum, but it's not attracting a lot of attention...

Mousius

This doesn't seem controversial, using a getter seems better than direct access in either case - the one thing I would suggest is using device_type and GetDeviceType rather than GetTargetDeviceType given the entity on which the property is placed is Target

kparzysz-quic · 2022-08-23T13:33:56Z

It started the way you described, but the Vulkan target defines a property "device_type", which is unrelated to the TVM's device type. I'd actually prefer to avoid the extra "target" word, but I'm not sure how do deal with the Vulkan name conflict. "Device type" is actually the term used by Vulkan docs, but maybe we can rename it in TVM...

Mousius

It started the way you described, but the Vulkan target defines a property "device_type", which is unrelated to the TVM's device type. I'd actually prefer to avoid the extra "target" word, but I'm not sure how do deal with the Vulkan name conflict. "Device type" is actually the term used by Vulkan docs, but maybe we can rename it in TVM...

Ahhh, makes sense to disambiguate it then 😸

areusch

thanks @kparzysz-quic and sorry i missed your original thread.

areusch · 2022-08-23T20:19:52Z

src/target/target.cc

 TVM_REGISTER_GLOBAL("target.TargetCurrent").set_body_typed(Target::Current);
 TVM_REGISTER_GLOBAL("target.TargetExport").set_body_typed(TargetInternal::Export);
 TVM_REGISTER_GLOBAL("target.WithHost").set_body_typed(TargetInternal::WithHost);
+TVM_REGISTER_GLOBAL("target.TargetGetDeviceType").set_body_typed([](const Target& target) {


do we need this or can we just use the node attr accessor?

This function will check the attribute first, and if it's not set, it will return the default. It also shortens the invocation of GetAttr. Let me know what you'd prefer.

areusch · 2022-08-23T20:21:27Z

include/tvm/target/target_kind.h

  void VisitAttrs(AttrVisitor* v) {
    v->Visit("name", &name);
-    v->Visit("device_type", &device_type);
+    v->Visit("default_device_type", &default_device_type);


this would be a convenient time to update a file e.g. tests/release/PRERELEASE_NOTES.md to indicate an "api change." what do you think about creating that file?

cc @driazati @tqchen @Mousius

sounds like a good idea that would make generating coherent release notes a lot easier (at least to highlight big changes). line items like this should be enough:

* #<pr number>: <description> * #12509: this does X changes API A from Y to Z

I think it's a good idea to create the PRERELEASE.md, the one question I have is why under tests? It doesn't seem related to testing 🤔

that's where we have other release scripts (for now: https://github.com/apache/tvm/tree/main/tests/scripts/release and https://discuss.tvm.apache.org/t/establishing-a-home-for-tvm-dev-tools/13212), but we could also just put it at the top level

@Mousius yeah it's more like just keeping everything together right now. when i get a moment i'll do the tvm dev tools refactor and move it somewhere more sensible :)

include/tvm/target/target_kind.h

areusch · 2022-08-23T20:27:30Z

include/tvm/target/target_kind.h

          .add_attr_option<Target>("host")                        \
-          .add_attr_option<Integer>("from_device")
+          .add_attr_option<Integer>("from_device")                \
+          .add_attr_option<Integer>("target_device_type")


wondering if we should be producing this from e.g. processing -mcpu rather than allowing it to be explicitly set by the user?

TargetKind doesn't have "mcpu", so it would be limited to specific kinds.

areusch · 2022-08-30T23:18:27Z

for context for the Community Meeting tomorrow: hexagon is a backend which effectively compiles code in the same way as the LLVM target, but which at runtime needs a different Device API. At compile time, there's no reason that hexagon needs to use a different target.build. However, at present, we tie the runtime device_type to the Target kind.

There are a few different options at least for how we could resolve this:

allow this PR through, which lets the user configure it. However, a user may not necessarily understand which device type to use, and there may exist cases where the target device type shouldn't be included in tuning logs.
leave device_type for hexagon to be kDLCPU, and override the CPU Device API via patching the runtime::Registry.
allow targets to be flexible in determining their device_type, and make llvm -mcpu=hexagonvNN choose hexagon as the device type. this avoids the need to patch the registry.

areusch · 2022-08-31T16:06:22Z

We discussed this in the TVM Community Meeting this morning. Here are notes:

All targets derive from a given target kind
Targets derived from the llvm target kind share common components but it is special cased in TVM
- LLVM targets are presumed to generate a shared library
Target kind’s also share a device type relationship, and for llvm this is exclusivel kDLCPU
Hexagon is an llvm target, however the HexagonDeviceAPI used for Hexagon is unique and cannot be supported by the CPUDeviceAPI
Desire: Use the llvm target for the compiler and codegeneration, but utilize a unique runtime DeviceAPI
- Q: (and possibly a unique DLDeviceType?)
- Right now to achieve this one has to override the global registration for a kDLCPU DeviceAPI with the HexagonDeviceAPI
This PR proposes that just like a target can derive it’s properties from a target kind, the target can override the default device type so that a target is not bound to a specific device (ie for llvm the user wouldn’t be required to utilize a kDLCPU device type for the backing runtime apis)
- GetTargetDeviceType is the proposed api in the PR to offer the ability for different target to provide their own DeviceAPI.
Are there other analogies
Andrew:
- Two things being coordinated
  - 1. A runtime module is produced (not contentious)
  - 1. The C++ DeviceAPI, this can change between targets
- Work has been done in the C-API to support different type of hardware runtimes for accelerators
  - Currently makes global function calls based on names with a device name prefix
Two places to extend the abstraction
Current approach in PR has the advantage of changing existing code
- If we were going to map multiple device types to multiple device api’s there would likely need to be more code changes.
The DeviceAPI is some what a metadata structure that is provided
Virtual device: goal to map between device type and particular subtarget
Making a change to allow a target to have it’s own target device type would require making some changes about virtual device subtargetting
KP: If this PR goes in, I would support it with Hexagon and then would try to address different the overuse of kDLCPU as host devices

kparzysz-quic · 2022-09-21T18:50:38Z

Hi @areusch ... So what should we do with this PR? I don't think there was any opposition to this, but we didn't really reach the final decision in the community meeting either...

Mousius

Hi @areusch ... So what should we do with this PR? I don't think there was any opposition to this, but we didn't really reach the final decision in the community meeting either...

My suggestion is that in lieu of a better solution, we land this and move forwards - we'll likely have to do a fair amount of breaking changes if we want to clean up the Target conundrum in future anyway.

areusch

on second read I am good with this PR. sorry to hold it up.

areusch · 2022-08-26T15:59:21Z

include/tvm/target/target_kind.h

  void VisitAttrs(AttrVisitor* v) {
    v->Visit("name", &name);
-    v->Visit("device_type", &device_type);
+    v->Visit("default_device_type", &default_device_type);


@Mousius yeah it's more like just keeping everything together right now. when i get a moment i'll do the tvm dev tools refactor and move it somewhere more sensible :)

areusch · 2022-09-29T21:48:15Z

@tvm-bot rerun

areusch · 2022-09-29T21:52:24Z

can you sync this one though and get it through CI again? usually we break main by submitting a test that passed CI weeks ago

…type Implement Target::GetTargetDeviceType (C++) or get_target_device_type (python) to get the device type (kDL...) for a given target. The attribute "target_device_type" can be used to override the default device type associated with the target kind.

…type (apache#12509) Implement Target::GetTargetDeviceType (C++) or get_target_device_type (python) to get the device type (kDL...) for a given target. The attribute "target_device_type" can be used to override the default device type associated with the target kind.

kparzysz-quic changed the title ~~[Draft][Target] Add Target::GetDeviceType, replace direct accesses to…~~ [Target] Add target_device_type attribute to override default device_type Aug 22, 2022

kparzysz-quic requested a review from areusch August 22, 2022 20:21

kparzysz-quic marked this pull request as ready for review August 22, 2022 20:22

kparzysz-quic requested review from Lunderberg and Mousius August 22, 2022 20:26

Mousius requested changes Aug 23, 2022

View reviewed changes

Mousius self-assigned this Aug 23, 2022

Mousius approved these changes Aug 23, 2022

View reviewed changes

areusch reviewed Aug 23, 2022

View reviewed changes

Mousius approved these changes Sep 26, 2022

View reviewed changes

areusch approved these changes Sep 29, 2022

View reviewed changes

Krzysztof Parzyszek added 4 commits September 29, 2022 15:36

Rename set_device_type -> set_default_device_type

ec05631

Add PRERELEASE_NOTES.md, describe the changes from this PR

479974d

Move PRERELASE_NOTES.md to the right directory

cc34a70

Mousius merged commit d4bf9ec into apache:main Sep 30, 2022

kparzysz-quic deleted the target-device-type branch January 10, 2023 16:29

leandron mentioned this pull request Feb 1, 2023

TVM v0.11.0 Release Candidate Notes #13899

Closed

[Target] Add target_device_type attribute to override default device_type #12509

[Target] Add target_device_type attribute to override default device_type #12509

Uh oh!

Conversation

kparzysz-quic commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kparzysz-quic commented Aug 23, 2022

Uh oh!

Mousius left a comment

Choose a reason for hiding this comment

Uh oh!

kparzysz-quic commented Aug 23, 2022

Uh oh!

Mousius left a comment

Choose a reason for hiding this comment

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

areusch commented Aug 30, 2022

Uh oh!

areusch commented Aug 31, 2022

Uh oh!

kparzysz-quic commented Sep 21, 2022

Uh oh!

Mousius left a comment

Choose a reason for hiding this comment

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

areusch commented Sep 29, 2022

Uh oh!

areusch commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kparzysz-quic commented Aug 20, 2022 •

edited

Loading