[Lowering] Add TargetDevice composite data structure. #8892

jroesch · 2021-08-31T22:58:13Z

This adds a new data structure which captures both the target and device as a composite data structure, see the comments for more motivation. The idea here is to enable us to track same device types with different targets. For example we could deal with ARM core with different targets for different CPUs, GPU, and NPU.

cc @junrushao1994 @areusch @mbs-octoml @csullivan @adstraw @manupa-arm @Mousius

electriclilies

LGTM, just one nit and a clarifying question about the longterm plan for Targets and Devices ---
Is idea of TargetDevice to replace both Target and Device? And if we do replace Target with TargetDevice, how will we indicate that the Target for an expression has been set but not the Device? Thanks :)

electriclilies · 2021-08-31T23:25:15Z

include/tvm/target/target_device.h

+ * \brief A compile time representation of a target device.
+ *
+ * This data structure consists of both the compiler target and a virtual device,
+ * a tvm::Device where the the identifier is a virtual identifier and a concrete


Can you expand on what you mean by a virtual identifier?

junrushao · 2021-08-31T23:43:15Z

Hey Jared, thanks for the PR!

I understand the design of the virtual device id and it looks good to me if there are some use cases of it (usually we just assume it’s 0).

On the other hand, there is direct correspondence between TargetKind to DLDeviceType, e.g. https://github.com/apache/tvm/blob/main/src/target/target_kind.cc#L278. So shall we just infer it from TargetNode::kind?

areusch · 2021-09-01T07:37:00Z

include/tvm/target/target_device.h

+ * a tvm::Device where the the identifier is a virtual identifier and a concrete
+ * device type.
+ *
+ * Executors are required to handle how to map virtual device identifiers to physical


clean up this sentence a bit:

Suggested change

* Executors are required to handle how to map virtual device identifiers to physical

* Before inference, executors should map virtual device identifiers (included in the executor config) to physical

device identifiers (e.g. DLDevice)

areusch · 2021-09-01T07:37:40Z

include/tvm/target/target_device.h

+ * \file tvm/target/target_device.h
+ * \brief A compile time representation of a target device.
+ *
+ * This data structure consists of both the compiler target and a virtual device,


this isn't a file-level comment and belongs on the docstring for TargetDevice

areusch · 2021-09-01T07:51:16Z

include/tvm/target/target_device.h

+ * Executors are required to handle how to map virtual device identifiers to physical
+ * device identifiers.
+ *
+ * The reason to introduce this data structure is that for much of compilation we


I kind of agree with this, but it requires a lot of other contextual information to understand things like the reach and purpose of this data structure. Specifically, it would be great to assume the reader does not understand the distinction between "concrete device" and "target" here (or explain it).

I think you could say something like:

This data structure defines a compile-time identifier for one of the devices specified in the Target. For instance, a Target string `cuda --target-host=llvm -mcpu=i386` requires the compiler to track two such devices: the GPU used by CUDA and the CPU targeted by LLVM. Before this data structure, the two different devices were configured by the runtime by finding a user-specified `DLDevice` whose `device_type` field matches the Target. More complex deployment scenarios can't be modeled coherently across compiler and runtime in this system without aliasing in the target string: * two of any `device_type` with differing capabilities * reconfigurable devices (e.g. FPGA) for which TVM requires two distinct configurations * devices with memory constraints (which need to be expressed in terms of a specific, but unresolved-at-compile-time DLDevice)

BTW I owe you all an RFC to both justify this and bring together the threads on target maps, default targets, and unified target/device/memory scope planning.

areusch · 2021-09-01T07:52:09Z

include/tvm/target/target_device.h

+ * device API actions.
+ *
+ * The idea is that we will carry around TargetDevice structures until device and
+ * target planning at which time we can inject explicit virtual devices in the


explain what "inject explicit virtual devices" means--are you placing this structure as an attribute in the program, or just the id, or ? How should this structure be identified, in general?

areusch · 2021-09-01T07:52:27Z

include/tvm/target/target_device.h

+ * different targets or compilation options, and eventually resolve to a phyical
+ * set of devices with code specialized using the correct target.
+ *
+ * For example consider mobile SoCs which may contain two CPU types, a mobile GPU,


i think just two CPU types is sufficient here

areusch · 2021-09-01T07:56:11Z

include/tvm/target/target_device.h

+
+namespace tvm {
+
+class TargetDevice;


TargetDevice makes sense in context of your previous explanation but not out of context. Can we pick a different name? I suggest we consider names that reflect the identifier used e.g. DeviceName or VirtualDevice or CompileTimeDevice.

I also prefer a string identifier so it can be given by the user or SDK authors. We can register these strings at runtime or produce enums if efficiency is a concern, but specifying memory layouts with respect to a virtual device id already seems obnoxious AF. I vastly prefer "dsp-cpu" over 1.

Potentially it's worth adding a default attribute to Target which is alias rather than adding it to this structure?

areusch · 2021-09-01T07:57:10Z

include/tvm/target/target_device.h

+    v->Visit("target", &target);
+    v->Visit("virtual_device_id", &virtual_device_id);
+    DLDeviceType* ptr = &device_type;
+    v->Visit("device_type", reinterpret_cast<int*>(ptr));


areusch · 2021-09-01T07:58:28Z

On the other hand, there is direct correspondence between TargetKind to DLDeviceType, e.g. https://github.com/apache/tvm/blob/main/src/target/target_kind.cc#L278. So shall we just infer it from TargetNode::kind?

@junrushao1994 how do you propose to infer the two-different-CPUs or two-different-FPGAs case? Also see my comments on the commentary at the top of target_device.cc. My thoughts are that being explicit at this level will provide a better user experience. I don't think it removes our ability to infer devices based on Target when there is no ambiguity, and it provides us a language in which we can warn the user when ambiguity exists.

Mousius · 2021-09-01T16:26:36Z

src/target/target.cc

@@ -17,7 +17,7 @@
 * under the License.
 */
 /*!
- *  Compile executable modules.
+ * \brief Implementation of methods, and FFI interfaces for the compilation target object.


Really minor nit, can you do \file then \brief ?

Mousius · 2021-09-01T16:28:09Z

src/target/target_device.cc

+ * under the License.
+ */
+/*!
+ * \brief The implementation of the TargetDevice object for representing compilation target + virtual device.


Similarly here, re-order to \file then \brief

Mousius · 2021-09-01T16:33:12Z

python/tvm/relay/transform/transform.py

-    """Annotate ops in an experession with a provied compiler/target and then
-    use it for codegen.
+    """
+    The annotate the operations in an expression with the provided compiler target


Suggested change

The annotate the operations in an expression with the provided compiler target

Annotate the operations in an expression with the provided compiler target,

Mousius · 2021-09-01T16:33:38Z

src/relay/transforms/fold_scale_axis.cc

@@ -204,6 +204,10 @@ using FForwardRewrite = TypedPackedFunc<Expr(const Call& ref_call, const Array<E
 //----------------------------------------------
 class ForwardPrep : private MixedModeVisitor {
 public:
+  // This is needed to silence a clang-warning about not correclty detecting


Suggested change

// This is needed to silence a clang-warning about not correclty detecting

// This is needed to silence a clang-warning about not correctly detecting

Mousius · 2021-09-01T16:38:52Z

Is idea of TargetDevice to replace both Target and Device? And if we do replace Target with TargetDevice, how will we indicate that the Target for an expression has been set but not the Device? Thanks :)

On the other hand, there is direct correspondence between TargetKind to DLDeviceType, e.g. https://github.com/apache/tvm/blob/main/src/target/target_kind.cc#L278. So shall we just infer it from TargetNode::kind?

These two comments together make me wonder if the correct mapping is directly from Target -> Device and from TargetKind to DeviceType rather than introducing a new structure?

manupak · 2021-10-01T08:15:20Z

Hi @jroesch ,

I finally manage to get some time to read this :). I think code looks generally good modulo the comments.

I have a broad design question.

At which phase, would we be doing the partitioning of devices for same kind ?
Confusingly, at the minute we have two annotation (one in the BYOC pipeline) and one internally in the relay lowering pipeline.
[Related PRs and discussion : https://github.com//pull/7428 and https://discuss.tvm.apache.org/t/rfc-composite-target/7744/10? -- cc: @mbs-octoml ]

The strategy we are using in the annotation target pass (of the BYOC pipeline -- it does not really need to be just a BYOC thing), is we greedily partition the Relay OPs given the knowledge that some target kinds are better in processing them.

Therefore, this is a step beyond that where we have to deal with multiple devices of same kind and brings algorithmic problem of load balancing between devices. I appreciate that it can be a complex problem; however, my immediate question is what is the initial strategy that is being planned to use this from an annotation point of view ?

mbs-octoml · 2021-10-01T14:24:48Z

The RFC I promised in #8892 (comment) is now at up at apache/tvm-rfcs#38

mbs-octoml · 2021-10-08T22:46:46Z

I've merged this PR into my exploration and suggest we withdraw it from here.

areusch · 2021-10-12T20:48:22Z

@mbs-octoml seems you are taking this on so i will close this PR. @jroesch can feel free to reopen if he has cycles to push it through.

jroesch added 2 commits August 26, 2021 14:29

Add initial scaffolding of target_device.h

5f6a395

Clean up some badly written comments and impl target_device.cc

55b4f3a

jroesch requested review from anijain2305, junrushao, jwfromm, kparzysz-quic, MarisaKirisame, masahi, mbrookhart, slyubomirsky, tqchen, vinx13, wweic, yzhliu, zhiics and ZihengJiang as code owners August 31, 2021 22:58

jroesch removed request for wweic, yzhliu, masahi, tqchen, MarisaKirisame, mbrookhart, slyubomirsky, vinx13, jwfromm, anijain2305, ZihengJiang and kparzysz-quic August 31, 2021 23:00

electriclilies approved these changes Aug 31, 2021

View reviewed changes

areusch requested changes Sep 1, 2021

View reviewed changes

Mousius requested changes Sep 1, 2021

View reviewed changes

This was referenced Sep 20, 2021

[Bug] BuiltinLower has hard-coded heuristic for alloca which is not appropriate for all kDLCPU target devices #9022

Closed

C Device API apache/tvm-rfcs#31

Merged

manupak mentioned this pull request Oct 1, 2021

[RFC] Unified device/target/memory scope planning apache/tvm-rfcs#38

Closed

areusch mentioned this pull request Oct 3, 2021

Command Line Composition from Internal Registry apache/tvm-rfcs#28

Merged

areusch closed this Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lowering] Add TargetDevice composite data structure. #8892

[Lowering] Add TargetDevice composite data structure. #8892

jroesch commented Aug 31, 2021

electriclilies left a comment

electriclilies Aug 31, 2021

junrushao commented Aug 31, 2021

areusch Sep 1, 2021

areusch Sep 1, 2021

areusch Sep 1, 2021

mbs-octoml Sep 1, 2021

areusch Sep 1, 2021

areusch Sep 1, 2021

areusch Sep 1, 2021

Mousius Sep 1, 2021

areusch Sep 1, 2021

areusch commented Sep 1, 2021 •

edited

Loading

Mousius Sep 1, 2021

Mousius Sep 1, 2021

Mousius Sep 1, 2021

Mousius Sep 1, 2021

Mousius commented Sep 1, 2021 •

edited

Loading

manupak commented Oct 1, 2021

mbs-octoml commented Oct 1, 2021

mbs-octoml commented Oct 8, 2021

areusch commented Oct 12, 2021

	* Executors are required to handle how to map virtual device identifiers to physical
	* Before inference, executors should map virtual device identifiers (included in the executor config) to physical
	device identifiers (e.g. DLDevice)

	The annotate the operations in an expression with the provided compiler target
	Annotate the operations in an expression with the provided compiler target,

	// This is needed to silence a clang-warning about not correclty detecting
	// This is needed to silence a clang-warning about not correctly detecting

[Lowering] Add TargetDevice composite data structure. #8892

[Lowering] Add TargetDevice composite data structure. #8892

Conversation

jroesch commented Aug 31, 2021

electriclilies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junrushao commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areusch commented Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mousius commented Sep 1, 2021 • edited Loading

manupak commented Oct 1, 2021

mbs-octoml commented Oct 1, 2021

mbs-octoml commented Oct 8, 2021

areusch commented Oct 12, 2021

areusch commented Sep 1, 2021 •

edited

Loading

Mousius commented Sep 1, 2021 •

edited

Loading