You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/05/17 18:38:15 UTC
[GitHub] [tvm] apeskov opened a new pull request, #11345: [DNNL] Add TensorRequisite concept. Multi instance support
apeskov opened a new pull request, #11345:
URL: https://github.com/apache/tvm/pull/11345
**Summary**
There are several limitation which prevent DNNL runtime from using in multi instance mode. This patch tries to eliminate some of them.
Multi instance mode means: call Run() method concurrently from several threads for single instance of DNNLJSONRuntime.
Particularly changed for multi instance support:
* Do not modify DNNLJSONRuntime fields from Run method. Make it like a "const" method.
* Use explicit dnnl scratchpads where it requested.
* Make Intermediate tensors collection individual for each thread.
Other improvements:
* Zero copy handling of Input/Output tensors
* Use query API to ask DNNL about desired layouts. Prevent from using of unoptimised kernels.
* Automatic injection or reorder primitives if layout doesn't match.
* Support of different data types (int8, uint8, int32). Eliminate all "fp32" hardcoded values.
**Details**
Introduced indirect handling of memory objects. New objects are TensorRequisite and TensorRegistry.
TensorRequisite - describe sequence of transformation on top of some source tensor.
TensorRegistry - implement matching of TensorRequisite described tensor and real dnnl::memory objects.
This concept allows to:
* Decouple primitives arguments and real memory object. Matching of arguments to memory objects happens on demand depending on contexts of execution (thread id, arguments of method Run).
* Don't care about nature of tensor. Constant weights, intermediate tensors and external input/output processed identically.
Some pseudo code to demonstrate concept:
```
DLTensor src_mem = {5, 2, 128, 128, 8} // 5D tensor
// Describe sequence of layout transformation
auto tr = TensorRequisite.AsIs(src_mem, eid); // 5D
tr = tr.treatAs("ABCD8b"); // 4D
tr = tr.permute({0, 2, 3, 1}); // permute axes NCHW -> NHWC
tr = tr.crop({1, 128, 128, 16}, {0, 0, 0}); // extract first batch
tr = tr.squeeze();
// register TR
TensorRegistry t_reg;
auto t_id = t_reg.register(tr);
t_reg.finalize();
// Obtain dnnl::memory object inside of method Run()
auto mem = t_id.solve(t_id, ext_io_provider);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] apeskov commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
apeskov commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1129197672
@masahi Please take a look. This is a part of changes extracted from https://github.com/apache/tvm/pull/9618.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1133298471
Can you make the coding style consistent with the rest of the codebase? We are not supposed to use `camelCase`. I can tolerate if there are only a few of them, but I feel this change contains too much of your own style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] apeskov commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
apeskov commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1142221215
@masahi, I'm slightly confused with coding style.
In PR https://github.com/apache/tvm/pull/11111 you approved a new function [dtype_dl2dnnl](https://github.com/apache/tvm/blob/efec735626cb2932df7771bdff60c2e9ff1b14da/src/runtime/contrib/dnnl/dnnl_utils.h#L41). This is definitely not a `CamelCase` naming.
In my patch I would like to add several more similar util functions. And what naming I should use? `snake_case` like previous one or `CamelCase` like you requested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] apeskov commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
apeskov commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1144642500
@masahi Thank you for explanation about mandatory and recommendation part of code style.
PR turned green. You can continue reviewing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1137153403
Yes, sorry I meant we should use `UpperCamelCase` instead of `lowerCamelCase`. I saw many `lowerCamelCase` in this PR. Since this is a runtime code for one backend and not like the core code, I'd say we don't have to be too strict with this rule. But in general, I think it's better to stick with the convention, unless there is a good reason to deviate (e.g. match coding style with dnnl etc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] apeskov commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
apeskov commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1137129389
Yes, I can. Could you please indicate particular places you treat as violation of coding style? I was trying to keep original code style as much as possible.
> We are not supposed to use `camelCase`
This statement slightly contradicts with TVM recommendation (Google style) and other TVM code base. Do you mean I should use `UpperCamelCase` instead of "lowerCamelCase"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1138261289
There is a conflict due to the merge of https://github.com/apache/tvm/pull/11111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1143184845
Yeah, how about this: I don't think using `CamelCase` should be mandatory for all functions. As a general rule of thumb, we should use `CamelCase` by default, but if a person is aware of this general convention but still wants to use `snake_case` for some reasons, I'd say go ahead. In particular, since DNNL uses `snake_case`, I understand that making some util functions be `snake_case` can make the code more natural.
I brought this up just because I saw a lot of uses of the style like `makeSolver` in this PR, which seemed arbitrary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #11345:
URL: https://github.com/apache/tvm/pull/11345#issuecomment-1129255608
Please fix the build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #11345: [DNNL] Add TensorRequisite concept. Multi instance support
Posted by GitBox <gi...@apache.org>.
masahi merged PR #11345:
URL: https://github.com/apache/tvm/pull/11345
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org