[RFC] Further Unify Packed and Object in TVM Runtime #97

tqchen · 2023-01-08T17:16:27Z

This RFC proposes to further unify our PackedFunc and Object in TVM Runtime. The proposal builds on top of our past lessons and recont lessons from related project such as matxscript

The key improvements include: unifying type_code, solidifying AnyValue support for both stack and object values, open doors for small-string and NLP-preprocessing, and enable universal container.

rendered

tqchen · 2023-01-08T17:21:51Z

cc @apache/tvm-committers for visibility

See also the related forum posts

Co-authored-by: Xiandi Ma <maxiandi@bytedance.com> Co-authored-by: Junru Shao <junrushao1994@gmail.com>

leandron

Hi @tqchen et al. This change seems positive as consolidating the generic object type. I've got a few minor points and I guess my main question which asking for a slight expansion on how the change is specifically expected to impact TVM runtimes.

leandron · 2023-01-09T10:55:58Z

rfcs/0097-unify-packed-and-object.md

+
+## Drawbacks
+
+The design proposal would involve changes in our runtime system. This is however, a positive step to further solidy and reduce the overall amount of concepts in the code-base, further unify packed and object, simplify and solidify our implementation along side of AnyValue and Object. Finally, it also opens doors for NLP applications and cross project interactions.


Finally, it also opens doors for NLP applications and cross project interactions.

This is a feature, not really a problem or something which would render this proposal as less acceptable, so perhaps this specific point can be removed.

rfcs/0097-unify-packed-and-object.md

leandron · 2023-01-09T11:07:12Z

rfcs/0097-unify-packed-and-object.md

+
+Additionally, Unifying FFI and Object would bring further unification and reduction in our overall code complexity while leveling up the extensibility, so it serves as a strong improvement to the overall quality of the project. 
+
+## Drawbacks


Can you comment on how specifically the proposed change impacts the TVM runtime, e.g. is this a breaking change in some aspect?

Please see M0 section in the Phasing part. WIll also add a reference here:

Note that this implies (by intention) change the ABI of packed func. We will update compiler/runtime to do so.

All front-end, compiler, runtime will be updated together to ensure the current testcases continue to pass.

We will introduce adapter to support the TVMArgs during the transition, but favors moving to new state to reduce overall complexity.

More details can be found in D4

Updated the text to refer to the Phasing section

leandron · 2023-01-09T11:38:20Z

rfcs/0097-unify-packed-and-object.md

+
+### N0: First class stack small string and AnyValue
+
+**Lesson from matxscript:** Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can common get the one of the programs below


We can common get the one of the programs below

Not sure I understand this part, can you rephrase?

Rephrased a bit

leandron · 2023-01-09T11:42:13Z

rfcs/0097-unify-packed-and-object.md

+
+## Drawbacks
+
+The design proposal would involve changes in our runtime system. This is however, a positive step to further solidy and reduce the overall amount of concepts in the code-base, further unify packed and object, simplify and solidify our implementation along side of AnyValue and Object. Finally, it also opens doors for NLP applications and cross project interactions.


Does this affects and cover all existing TVM runtimes? If not, please specify.

We do intended to do a refactor for all of them for consistency and as believe such design can be positive for all tvm runtimes and in this case can be reasonably done together.

It will not affect unpacked ABI which is more frequently use in micro and another ABI that is not part of packed

Mousius

Hi @tqchen et al,

Looks like a good unification which will hopefully solve a bunch of issues, one specific one I'd like to highlight for you is where we're getting the wrong types out of the type system in tvmc (IntImm is both int and bool):
https://github.com/apache/tvm/blob/main/python/tvm/driver/tvmc/target.py#L37-L41
Am I assuming correctly that by using AnyValue and allowing it to map to the correct Python type we'll be able to properly infer the values here?

I've posted a few questions alongside a general suggestion that we orient this towards improving TVM with a reference to matxscript kept in Prior Art for reference rather than through-out. Otherwise most of my comments are minor spelling mistakes - it might be worth running this through a spell checker?

Mousius · 2023-01-09T10:47:38Z

rfcs/0097-unify-packed-and-object.md

+
+### N0: First class stack small string and AnyValue
+
+**Lesson from matxscript:** Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can common get the one of the programs below


Put the reference to matxscript in Prior Art, the statement stands on its own.

Suggested change

**Lesson from matxscript:** Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can common get the one of the programs below

Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can commonly get one of the programs below:

This particular lesson do come from matxscript more significantly :) Including the unicode example. So I think it is approperiate to cite in both places

Rephrased here

Mousius · 2023-01-09T10:50:56Z

rfcs/0097-unify-packed-and-object.md

+
+- Need a base AnyValue to support both stack values and object.
+    - This is to provide a safety net of translation.
+- The AnyValue needs to accommodate small-string(on stack) to enable fast string processing. Specifically, note that the particular example creates a `UCS4String res` for every character of the word. If we run heap allocation for each invocation, or even do reference countings, this can become expensive.


Does this generalise to AnyValue needing to accommodate stack values rather than specifically small strings?

Yes This does generalizes to need to support stack values.
However, that also depends on whether such scenarios are useful in the settings we consider. In this particular case. We have not yet seen needs on other stack values yet. But the design naturally generalizes to stack values

will add a note here

Mousius · 2023-01-09T10:52:42Z

rfcs/0097-unify-packed-and-object.md

+// Note that word[i] returns a UCS4String container to match python semantics 
+// Use UCS4String stores unicode in a fixed-length 4 bytes value to ease random
+// access to the elements. 
+List unicode_split(const UCS4String& word) {


Could this be List<UCS4String> so we type the items also?

Yes, either way can happen

added a note about this

rfcs/0097-unify-packed-and-object.md

Mousius · 2023-01-09T13:38:20Z

rfcs/0097-unify-packed-and-object.md

+
+    This approach combines code and value into a single entity, this would mean a change of ABI convention in generated code. This approach makes it possible to directly return a small-str without boxing it (however if it is faced in python frontend, likely we still need to box it to str).
+
+    We will introduce adapters to support the old calling convention, which constructs TVMAnyView on the stack then pass things over in the transitioning period.


What's the planned naming conventions for this and how do they eventually get transitioned to just being TVMCPackedFunc again?

Will do an update on this.

Added an expanded explaination

rfcs/0097-unify-packed-and-object.md

tqchen · 2023-01-09T15:52:43Z

Thanks @leandron @Mousius for helpful suggestions in readability, we have incorporated most of them. We would like still keep discussions of prior work design(in this case matx) both in prior Art as well as in the section of respective context to give clarity and context for the reader (this is also a common practice in technical scientific writing). Also updated the text with more clarifications on some of the questions asked.

rfcs/0097-unify-packed-and-object.md

Mousius · 2023-01-09T17:22:48Z

rfcs/0097-unify-packed-and-object.md

+- T3: AnyPad: an any value that have larger padded size to accomodate on stack values.
+    - When the initial value defaults to null. Both AnyValue and AnyPad, can choose to fill the small_len to be the size of total bytes available. This can help us to be able to pass small string back in C API (without template), by looking at `AnyValue*` ’s small_len field to decide the maximum bytes allowed.
+
+**Discussions**  The default size of AnyValue is 16 bytes. This means that for small string, we can use extra 8 bytes to store the string part(7 bytes if we need a tail `\0`). If we go with UCS4, we can store two extra UCS4 Char without the tail `\0`. The extra space may not be sufficient for some of the small string needs (as a reference matxscript adopts extra padding of 8 bytes to accommodate small string unicode). AnyPad serves as another variation of AnyValue that contains extra stack space. AnyPad is intended to be used interchangeably in any places that AnyValue appears. See also followup sections on conversions function signatures on how that works. One interesting future direction point here is that future compilers can choose to try different AnyPad in code generation and autotune the padding default to the scenario that best fits the application.


The future reference is unnecessary to motivate the current work and you've copied it to Future Possibilities, therefore I still advocate for removing it here.

I removed that per suggestion

rfcs/0097-unify-packed-and-object.md

tqchen · 2023-01-09T18:13:13Z

@Mousius to specifically answer your questions on IntImm for bool and int. Likely this is due to the way we use container and checkers. Right now IntImm is indeed designed to hold for int and bool. And we will need to specific type checking on dtype which might be missed in such a case. Differentiating bool and int in the special section will likely resolve the issue if we believe it is important to have special casing for bool

tqchen · 2023-01-15T14:16:16Z

@leandron @Mousius please take another look, i believe all the outstanding comments are addressed

cyx-6

LGTM! We are so excited to embrace a more unified and robust runtime object system in TVM. I believe this system will not only help address some current issues, but also be a solid foundation of our TVM Unity. And hope to have some contribution with our new runtime object system implementation. :)

tqchen · 2023-02-04T15:26:59Z

ping @Mousius @leandron

Mousius · 2023-02-06T09:10:40Z

@tqchen Ack. I'll take another look this week.

Hzfengsy · 2023-02-15T14:29:44Z

The comments so far seem have been addressed, would love to see if we have additional comments, and move forward on this

Hzfengsy · 2023-02-27T04:55:57Z

Thanks for everyone's input. We are going to merge in 24 hours if there are no additional comments.

Hzfengsy · 2023-02-28T08:33:07Z

This RFC is merged now. Thanks @tqchen for the proposal and the reviews from @leandron @Mousius @cyx-6

tqchen force-pushed the unify-packed branch from 8421ac7 to 708fbf7 Compare January 8, 2023 17:19

Add RFC unify packed and object

a4c626c

Co-authored-by: Xiandi Ma <maxiandi@bytedance.com> Co-authored-by: Junru Shao <junrushao1994@gmail.com>

tqchen force-pushed the unify-packed branch from 708fbf7 to a4c626c Compare January 8, 2023 18:36

leandron reviewed Jan 9, 2023

View reviewed changes

Mousius requested changes Jan 9, 2023

View reviewed changes

Address review comments

4a18791

tqchen force-pushed the unify-packed branch from c1f3a43 to 4a18791 Compare January 9, 2023 15:49

Mousius requested changes Jan 9, 2023

View reviewed changes

Update per suggestion

6190d8c

cyx-6 approved these changes Feb 1, 2023

View reviewed changes

Mousius approved these changes Feb 9, 2023

View reviewed changes

Hzfengsy merged commit d646a22 into apache:main Feb 28, 2023


		## Drawbacks

		The design proposal would involve changes in our runtime system. This is however, a positive step to further solidy and reduce the overall amount of concepts in the code-base, further unify packed and object, simplify and solidify our implementation along side of AnyValue and Object. Finally, it also opens doors for NLP applications and cross project interactions.


		Additionally, Unifying FFI and Object would bring further unification and reduction in our overall code complexity while leveling up the extensibility, so it serves as a strong improvement to the overall quality of the project.

		## Drawbacks


		### N0: First class stack small string and AnyValue

		Lesson from matxscript: Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can common get the one of the programs below

	Lesson from matxscript: Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can common get the one of the programs below
	Data preprocessing is an important part of ML pipeline. Pre-processing in NLP involves strings and containers. Additionally, when translating programs written by users (in python), there may not be sufficient type annotations. We can commonly get one of the programs below:


		This approach combines code and value into a single entity, this would mean a change of ABI convention in generated code. This approach makes it possible to directly return a small-str without boxing it (however if it is faced in python frontend, likely we still need to box it to str).

		We will introduce adapters to support the old calling convention, which constructs TVMAnyView on the stack then pass things over in the transitioning period.

[RFC] Further Unify Packed and Object in TVM Runtime #97

[RFC] Further Unify Packed and Object in TVM Runtime #97

Conversation

tqchen commented Jan 8, 2023 • edited

tqchen commented Jan 8, 2023 • edited

leandron left a 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

tqchen Jan 9, 2023 • edited

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

tqchen Jan 9, 2023 • edited

Choose a reason for hiding this comment

Mousius left a comment • edited

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

tqchen Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jan 9, 2023

tqchen commented Jan 15, 2023

cyx-6 left a comment

Choose a reason for hiding this comment

tqchen commented Feb 4, 2023

Mousius commented Feb 6, 2023

Hzfengsy commented Feb 15, 2023

Hzfengsy commented Feb 27, 2023

Hzfengsy commented Feb 28, 2023

tqchen commented Jan 8, 2023 •

edited

tqchen commented Jan 8, 2023 •

edited

tqchen Jan 9, 2023 •

edited

tqchen Jan 9, 2023 •

edited

Mousius left a comment •

edited

tqchen Jan 9, 2023 •

edited

tqchen Jan 9, 2023 •

edited