You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "adstraw (via GitHub)" <gi...@apache.org> on 2023/03/09 18:52:03 UTC
[GitHub] [tvm] adstraw opened a new pull request, #14254: [Hexagon] Add concept of DMA groups
adstraw opened a new pull request, #14254:
URL: https://github.com/apache/tvm/pull/14254
Add the concept of DMA group to align Hexagon User DMA with existing async copy solutions e.g. nVidia. Allows for the grouping of one or more DMA copies into a group and tracking the group with "in flight" counts created by InjectSoftwarePipeline pass. For now, this allows for the removal of `merge_async_commit_queue_scope` which was a Hexagon specific workaround due to lack of support for grouping DMA copies. Later, in a future PR, this functionality may lead to the possibility of doing DMA copy on Hexagon over non-contiguous regions.
--
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] Lunderberg commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1139148547
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
Review Comment:
Nit: Should document behavior of nested `dma_start_group()`.
```c++
/* \brief Start a group of DMA copies
*
* Any call to `dma_copy()` that occur after `dma_start_group()` will
* be deferred until the next call to `dma_end_group()`, rather than
* being launched immediately.
*
* Only one DMA group may be active at a given time. Calling
* `dma_start_group()` while a group is already active is unsupported.
*/
```
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
Review Comment:
Nit: Can we explicitly specify the behavior, both for when a group is active and when a group is inactive?
```c++
/* \brief Initiate a non-blocking DMA copy from source to destination
*
* If a `dma_start_group()` call is active, the copy will be collected,
* and will be launched when the next `dma_end_group()` call occurs.
*
* If no `dma_start_group()` call is active, the copy will be launched
* immediately.
*/
```
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -233,6 +233,19 @@ TVM_REGISTER_GLOBAL("device_api.hexagon.dma_wait").set_body([](TVMArgs args, TVM
*rv = static_cast<int32_t>(0);
});
+TVM_REGISTER_GLOBAL("device_api.hexagon.dma_start_group")
+ .set_body([](TVMArgs args, TVMRetValue* rv) {
Review Comment:
Nit: When the input/output types are fixed, the `.set_body_typed()` method can be used to avoid needing manual argument wrangling.
```c++
.set_body_typed([](int queue_id) -> int32_t {
return HexagonDeviceAPI::Global()->UserDMA()->StartGroup(queue_id);
});
```
##########
src/tir/transforms/lower_async_dma.cc:
##########
@@ -22,26 +22,61 @@
*/
#include <tvm/arith/analyzer.h>
+#include <tvm/arith/bound.h>
#include <tvm/arith/iter_affine_map.h>
+#include <tvm/tir/analysis.h>
+#include <tvm/tir/buffer.h>
+#include <tvm/tir/stmt.h>
#include <tvm/tir/stmt_functor.h>
#include <tvm/tir/transform.h>
+#include <optional>
+
+#include "../../arith/ir_mutator_with_analyzer.h"
#include "ir_utils.h"
namespace tvm {
namespace tir {
-class AsyncDMALowerer : public StmtExprMutator {
+class AsyncDMALowerer : public arith::IRMutatorWithAnalyzer {
public:
- explicit AsyncDMALowerer(bool dma_bypass_cache) : dma_bypass_cache_(dma_bypass_cache) {}
+ explicit AsyncDMALowerer(bool dma_bypass_cache, arith::Analyzer* analyzer)
+ : IRMutatorWithAnalyzer(analyzer), dma_bypass_cache_(dma_bypass_cache) {}
+
+ Stmt VisitStmt_(const ForNode* loop) final {
+ // if for loop is not within async_commit_queue_scope
+ if (!async_queue_id_.has_value()) {
+ return arith::IRMutatorWithAnalyzer::VisitStmt_(loop);
+ }
- // Create member statement to track a mapping from iter var to iter range
- Stmt VisitStmt_(const ForNode* op) final {
- input_iters.Set(op->loop_var, Range(op->min, op->extent));
- return StmtExprMutator::VisitStmt_(op);
+ // if for loop is not a memcpy of a contiguous region
+ std::optional<tvm::tir::MemCpyDetails> mem_copy = IdentifyMemCpy(GetRef<For>(loop), analyzer_);
+ if (!mem_copy.has_value() || mem_copy->dest->region.size() != 1 ||
+ mem_copy->source->region.size() != 1) {
+ LOG(FATAL) << "Unable to lower async dma due to non contiguous memory access";
+ }
+
+ // now that we are about to perform the `copy` transform
+ // save queue ID for inspection in `wait` transform
+ // and, increment the number of DMA copies in the group
+ queue_ids_.insert(async_queue_id_.value());
+ dmas_in_group_++;
+
+ tvm::PrimExpr src_min = mem_copy->source->region[0]->min;
+ tvm::PrimExpr dst_min = mem_copy->dest->region[0]->min;
+ tvm::PrimExpr dst_extent = mem_copy->dest->region[0]->extent;
+
+ auto src = BufferLoad(mem_copy->source->buffer, {src_min});
+ auto dst = BufferLoad(mem_copy->dest->buffer, {dst_min});
+ return Evaluate(
+ Call(DataType::Int(32), builtin::dma_copy(),
+ {async_queue_id_.value(), Call(DataType::Handle(), builtin::address_of(), {dst}),
+ Call(DataType::Handle(), builtin::address_of(), {src}),
+ dst_extent * src->dtype.bytes(), dma_bypass_cache_}));
}
Stmt VisitStmt_(const AttrStmtNode* op) final {
+ arith::IRMutatorWithAnalyzer::VisitStmt_(op);
Review Comment:
Can we add a comment for why we need to pre-visit the `AttrStmt`?
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
+ * \brief Start a group of DMA copies
+ */
+TVM_DLL const Op& dma_start_group();
+
+/*!
Review Comment:
Similar here, we should document what usage is supported. This would also be a good place to specify that empty groups still count as "in-flight", since the `dma_copy()` in-between a start/end pair may have been optimized out.
```c++
/* \brief End a group of DMA copies
*
* Launch all calls to `dma_copy()` that occurred since the preceding
* `dma_start_group()`. Calling `dma_end_group()` without an active
* group is unsupported.
*
* Note: A group of DMA calls may be empty, and will still contribute
* to the count of in-flight groups used by `dma_wait()`.
*/
```
##########
src/tir/transforms/lower_async_dma.cc:
##########
@@ -63,22 +98,21 @@ class AsyncDMALowerer : public StmtExprMutator {
DLOG(INFO) << "AsyncDMALowerer exiting because the queue ID observed in the "
"`async_wait_queue_scope` transform has not been previously observed in the "
"`async_commit_queue_scope` transform";
- return StmtExprMutator::VisitStmt_(op);
+ return arith::IRMutatorWithAnalyzer::VisitStmt_(op);
Review Comment:
Do we need to re-visit with `IRMutatorWithAnalyzer` in this case? If the initial visiting is still valid, we should save the result and return it here.
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -103,15 +104,15 @@ int HexagonUserDMA::Copy(int queue_id, void* dst, void* src, uint32_t length, bo
return DMA_SUCCESS;
}
-void HexagonUserDMA::Wait(int queue_id, uint32_t max_dmas_in_flight) {
+void HexagonUserDMA::Wait(uint32_t queue_id, uint32_t max_dmas_in_flight) {
// wait (forever) until max DMAs in flight <= actual DMAs in flight
while (DMAsInFlight(queue_id) > max_dmas_in_flight) {
}
}
-uint32_t HexagonUserDMA::Poll(int queue_id) { return DMAsInFlight(queue_id); }
+uint32_t HexagonUserDMA::Poll(uint32_t queue_id) { return DMAsInFlight(queue_id); }
-uint32_t HexagonUserDMA::DMAsInFlight(int queue_id) {
+uint32_t HexagonUserDMA::DMAsInFlight(uint32_t queue_id) {
Review Comment:
Nit: Can we update the name from `DMAsInFlight` to `DMAGroupsInFlight`?
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -233,6 +233,19 @@ TVM_REGISTER_GLOBAL("device_api.hexagon.dma_wait").set_body([](TVMArgs args, TVM
*rv = static_cast<int32_t>(0);
});
+TVM_REGISTER_GLOBAL("device_api.hexagon.dma_start_group")
+ .set_body([](TVMArgs args, TVMRetValue* rv) {
+ int queue_id = args[0];
+ HexagonDeviceAPI::Global()->UserDMA()->StartGroup(queue_id);
+ *rv = static_cast<int32_t>(0);
Review Comment:
Should this use `uint32_t` for the `queue_id` and return value, to match the internal methods.
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140387258
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
+ * \brief Start a group of DMA copies
+ */
+TVM_DLL const Op& dma_start_group();
+
+/*!
Review Comment:
```
/*!
* \brief End a group of DMA copies
*
* Track all calls to `dma_copy()` that occurred since the preceding
* `dma_start_group()` as a single group in-flight.
*
* Calling `dma_end_group()` without an active group is unsupported.
*
* Note: A group of DMA calls may be empty, and will still contribute
* to the count of in-flight groups used by `dma_wait()`.
*/
```
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140368645
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
Review Comment:
Similarly, I ended up with this:
```
/*!
* \brief Initiate a non-blocking DMA copy from source to destination
*
* The copy is launched immediately.
*
* If a `dma_start_group()` call is active, the copy will be added
* to the current group for tracking of in-flight group counts.
*
* If no `dma_start_group()` call is active, the copy will be tracked
* individually i.e. as a group with size 1.
*/
```
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140366250
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
Review Comment:
Ended up with this:
```
/*!
* \brief Initiate a non-blocking DMA copy from source to destination
*
* The copy is launched immediately.
*
* If a `dma_start_group()` call is active, the copy will be added
* to the current group for tracking of in-flight group counts.
*
* If no `dma_start_group()` call is active, the copy will be tracked
* individually i.e. as a group with size 1.
*/
```
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140365973
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
Review Comment:
Good feedback. Groups are more about "tracking" than "launching" however.
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140366250
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
Review Comment:
Ended up with this:
```
/*!
* \brief Initiate a non-blocking DMA copy from source to destination
*
* If a `dma_start_group()` call is active, the copy will be added
* to the current group for tracking.
*
* If no `dma_start_group()` call is active, the copy will be tracked
* individually i.e. as a group with size one.
*/
```
--
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] Lunderberg merged pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg merged PR #14254:
URL: https://github.com/apache/tvm/pull/14254
--
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] tvm-bot commented on pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "tvm-bot (via GitHub)" <gi...@apache.org>.
tvm-bot commented on PR #14254:
URL: https://github.com/apache/tvm/pull/14254#issuecomment-1462595642
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @ibsidorenko <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140368645
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
Review Comment:
Similarly, I ended up with this:
```
/*!
* \brief Start a group of DMA copies
*
* Any call to `dma_copy()` that occurs after `dma_start_group()` will
* be added to the current group for tracking.
*
* Only one DMA group may be active at a given time. Calling
* `dma_start_group()` while a group is already active is unsupported.
*/
```
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140430525
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -233,6 +233,19 @@ TVM_REGISTER_GLOBAL("device_api.hexagon.dma_wait").set_body([](TVMArgs args, TVM
*rv = static_cast<int32_t>(0);
});
+TVM_REGISTER_GLOBAL("device_api.hexagon.dma_start_group")
+ .set_body([](TVMArgs args, TVMRetValue* rv) {
Review Comment:
Chose not to implement this for this iteration as it seems like we could / should redo the entire Hexagon Device API with this change.
--
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] adstraw commented on a diff in pull request #14254: [Hexagon] Add concept of DMA groups
Posted by "adstraw (via GitHub)" <gi...@apache.org>.
adstraw commented on code in PR #14254:
URL: https://github.com/apache/tvm/pull/14254#discussion_r1140368645
##########
include/tvm/tir/builtin.h:
##########
@@ -726,15 +726,26 @@ TVM_DLL const Op& texture2d_store();
TVM_DLL const Op& texture2d_load();
/*!
- * \brief Initiate a non-blocking DMA copy from source to destination
+ * \brief Initiate a non-blocking DMA copy from source to destination; a DMA copy outside of a group
+ * has a defacto group size of one
*/
TVM_DLL const Op& dma_copy();
/*!
- * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \brief Wait until the number of DMA groups in flight is less than or equal to some maximum
*/
TVM_DLL const Op& dma_wait();
+/*!
Review Comment:
Similarly, I ended up with this:
```
/*!
* \brief Start a group of DMA copies
*
* Any call to `dma_copy()` that occurs after `dma_start_group()` will
* be added to the current group for tracking of in-flight group counts.
*
* Only one DMA group may be active at a given time. Calling
* `dma_start_group()` while a group is active is unsupported.
*/
```
--
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