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 2021/06/29 10:54:38 UTC
[GitHub] [tvm] masahi opened a new pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
masahi opened a new pull request #8366:
URL: https://github.com/apache/tvm/pull/8366
--
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] vinx13 commented on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-879346109
Thanks @masahi @kparzysz-quic @csullivan @tqchen this is merged
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-871942607
@tqchen I've hit an issue in `lower_thread_allreduce.cc`. There are multiple uses of `AttrStmt` where a storage scope of a buffer variable is assigned after some complicated logic, like this:
https://github.com/apache/tvm/blob/ae58f2c387de9944d241a083ce9a0dd4c9ae613d/src/tir/transforms/lower_thread_allreduce.cc#L94
So I tried to replace this line by something like `SetStorageScope(repl->buffer_var, "shared")` but this doesn't work because I cannot get a mutable pointer to `PointerType` via `buffer_var->type_annotation.as<PointerTypeNode>()`.
Creating a new variable also doesn't work because the old `buffer_var` is [shared ](https://github.com/apache/tvm/blob/ae58f2c387de9944d241a083ce9a0dd4c9ae613d/src/tir/transforms/lower_thread_allreduce.cc#L354-L359) with other stmt nodes, so updating the `buffer_var` of `AllocateNode` results in the use of undefined variable in other nodes.
What should we do about this?
--
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] csullivan commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r668144726
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,91 @@
+/*
Review comment:
I think we need to also update `lower_device_storage_access_info.cc` to use the PointerType storage scope in this PR as well.
With a custom storage scope used in a cache_read stage, e.g.
```
s.cache_read(data, "global.texture", [OL])
```
I see that [lower_device_storage_access_info.cc#L72](https://github.com/apache/tvm/blob/main/src/tir/transforms/lower_device_storage_access_info.cc#L72) is unhappy with the scope tag.
```
Check failed: (e.info.defined()) is false: Cannot find memory info of global.texture
```
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,91 @@
+/*
Review comment:
I think we need to also update `lower_device_storage_access_info.cc` to use the PointerType storage scope in this PR as well.
With a custom storage scope used in a cache_read stage, e.g.
```
s.cache_read(data, "global.texture", [OL])
```
I see that [lower_device_storage_access_info.cc#L72](https://github.com/apache/tvm/blob/main/src/tir/transforms/lower_device_storage_access_info.cc#L72) is unhappy with the scope tag.
```
Check failed: (e.info.defined()) is false: Cannot find memory info of global.texture
```
and this ICHECK failure does not occur on main.
--
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 removed a comment on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi removed a comment on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-870508479
The CI job was killed during build twice for unclear reason.
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-870508479
The CI job was killed during build twice for unclear reason.
--
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] kparzysz-quic commented on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-875796073
Should we remove `scope` from `Buffer`? The scope is now available from the type annotation of the `data` member in `BufferNode`.
--
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] tqchen commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r664543848
##########
File path: src/target/llvm/codegen_llvm.h
##########
@@ -161,13 +161,6 @@ class CodeGenLLVM : public ExprFunctor<llvm::Value*(const PrimExpr&)>,
void VisitStmt_(const EvaluateNode* op) override;
protected:
- /*! \brief The storage information */
- struct StorageInfo {
- /*! \brief The storage scope */
- runtime::StorageScope scope;
- /*! \brief The alignment of allocation */
Review comment:
Let us keep the original data structure and leave alignment field here for now, in case we need some update in the future
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r665848790
##########
File path: python/tvm/script/special_stmt.py
##########
@@ -491,6 +491,22 @@ def var(dtype, span):
super().__init__(var, def_symbol=True)
+@register
+class BufferVarDef(SpecialStmt):
Review comment:
cc @spectrometerHBH for tvmscript changes. This is for distinguishing a normal variable from a buffer variable. The latter one requires storage scope. See the change in `test_tvmscript_roundtrip.py`. The tvm script printer was also updated.
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r665188307
##########
File path: src/tir/ir/stmt.cc
##########
@@ -61,6 +61,16 @@ TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable)
// AttrStmt
AttrStmt::AttrStmt(ObjectRef node, String attr_key, PrimExpr value, Stmt body, Span span) {
+ if (attr_key == attr::storage_scope) {
+ const VarNode* buf = node.as<VarNode>();
+ ICHECK(buf);
+ const auto* ptr_type = buf->type_annotation.as<PointerTypeNode>();
+ ICHECK(ptr_type) << "The provided variable is not of pointer type";
+ auto attr_scope = value.as<StringImmNode>()->value;
+ ICHECK(attr_scope == ptr_type->storage_scope)
+ << "Storage scopes attached to AttrStmt and buffer var are different. " << attr_scope
+ << ", " << ptr_type->storage_scope;
+ }
Review comment:
This is the invariant that enforces the same storage scope be attached to AttrStmt and buffer variables.
Later I'll send a follow up PR to remove `attr::storage_scope` completely.
cc @tqchen @junrushao1994 @csullivan
--
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] kparzysz-quic commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r666947190
##########
File path: src/runtime/thread_storage_scope.h
##########
@@ -118,7 +118,9 @@ struct StorageScope {
*/
static StorageScope Create(const std::string& s) {
StorageScope r;
- if (s.compare(0, 6, "global") == 0) {
+ if (s == "") {
Review comment:
I suggest using `s.empty()`.
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file update_pointer_storage_scope.cc
+ * \brief A pass to update storage scopes for buffer variables.
+ */
+#include "update_pointer_storage_scope.h"
+
+#include <tvm/tir/expr.h>
+#include <tvm/tir/op.h>
+#include <tvm/tir/stmt_functor.h>
+#include <tvm/tir/transform.h>
+
+#include <unordered_map>
+
+#include "../../runtime/thread_storage_scope.h"
+#include "ir_utils.h"
+
+namespace tvm {
+namespace tir {
+
+Var WithStorageScope(const VarNode* buffer_var, String storage_scope) {
+ auto* ptr_type = buffer_var->type_annotation.as<PointerTypeNode>();
+ ICHECK(ptr_type) << "The provided variable is not of pointer type";
+ return Var(buffer_var->name_hint, PointerType(ptr_type->element_type, storage_scope),
+ buffer_var->span);
+}
+
+UpdatePointerStorageScope::UpdatePointerStorageScope(
+ const std::unordered_map<const VarNode*, String>& new_storage_scopes) {
+ for (auto kv : new_storage_scopes) {
+ new_var_remap_[kv.first] = WithStorageScope(kv.first, kv.second);
+ }
+}
+
+PrimExpr UpdatePointerStorageScope::VisitExpr_(const VarNode* op) {
+ auto it = new_var_remap_.find(op);
+ if (it == new_var_remap_.end()) {
+ return GetRef<Var>(op);
+ }
+ return it->second;
+}
+
+PrimExpr UpdatePointerStorageScope::VisitExpr_(const LoadNode* op) {
+ auto remapped = StmtExprMutator::VisitExpr(op->buffer_var);
+ return Load(op->dtype, Downcast<Var>(remapped), StmtExprMutator::VisitExpr(op->index),
+ StmtExprMutator::VisitExpr(op->predicate));
+}
+
+Stmt UpdatePointerStorageScope::VisitStmt_(const AttrStmtNode* op) {
+ using runtime::StorageScope;
Review comment:
You never use this name in this function.
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file update_pointer_storage_scope.cc
+ * \brief A pass to update storage scopes for buffer variables.
+ */
+#include "update_pointer_storage_scope.h"
+
+#include <tvm/tir/expr.h>
+#include <tvm/tir/op.h>
+#include <tvm/tir/stmt_functor.h>
+#include <tvm/tir/transform.h>
+
+#include <unordered_map>
+
+#include "../../runtime/thread_storage_scope.h"
+#include "ir_utils.h"
+
+namespace tvm {
+namespace tir {
+
+Var WithStorageScope(const VarNode* buffer_var, String storage_scope) {
+ auto* ptr_type = buffer_var->type_annotation.as<PointerTypeNode>();
+ ICHECK(ptr_type) << "The provided variable is not of pointer type";
+ return Var(buffer_var->name_hint, PointerType(ptr_type->element_type, storage_scope),
+ buffer_var->span);
+}
+
+UpdatePointerStorageScope::UpdatePointerStorageScope(
+ const std::unordered_map<const VarNode*, String>& new_storage_scopes) {
+ for (auto kv : new_storage_scopes) {
Review comment:
Use `auto&`.
--
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] vinx13 commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r667362619
##########
File path: src/te/operation/cross_thread_reduction.cc
##########
@@ -1,3 +1,4 @@
+
Review comment:
remove this line
##########
File path: src/te/operation/cross_thread_reduction.cc
##########
@@ -177,7 +178,8 @@ Stmt MakeCrossThreadReduction(const ComputeOpNode* self, const Stage& stage,
std::vector<Var> res_handles(size);
for (size_t idx = 0; idx < size; ++idx) {
DataType dtype = reduces[idx]->dtype;
- res_handles[idx] = Var("reduce_temp" + std::to_string(idx), PointerType(PrimType(dtype)));
+ res_handles[idx] =
Review comment:
shall we remove `storage_scope` attr here https://github.com/apache/tvm/blob/f459dbfd3a09e5bcba25fd6838cf878efb961fa7/src/te/operation/cross_thread_reduction.cc#L229
https://github.com/apache/tvm/blob/f459dbfd3a09e5bcba25fd6838cf878efb961fa7/src/te/operation/cross_thread_reduction.cc#L234
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r664942125
##########
File path: src/tir/ir/buffer.cc
##########
@@ -45,12 +45,27 @@ Array<PrimExpr> SimplifyArray(arith::Analyzer* ana, Array<PrimExpr> array) {
return array;
}
-Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, Span span) {
+Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, String storage_scope,
+ Span span) {
DataType storage_dtype = (dtype == DataType::Bool() ? DataType::Int(8) : dtype);
- return Buffer(Var(name, PointerType(PrimType(storage_dtype)), span), dtype, shape,
+ return Buffer(Var(name, PointerType(PrimType(storage_dtype), storage_scope), span), dtype, shape,
Array<PrimExpr>(), PrimExpr(), name, "", 0, 0, kDefault, span);
}
+String GetStorageScope(Var buffer_var) {
Review comment:
Since this function is used a lot in `src/tir/transforms` and `src/tir/target`, I moved it to `src/tir/transforms/ir_utils.h`.
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r667585460
##########
File path: src/te/operation/cross_thread_reduction.cc
##########
@@ -177,7 +178,8 @@ Stmt MakeCrossThreadReduction(const ComputeOpNode* self, const Stage& stage,
std::vector<Var> res_handles(size);
for (size_t idx = 0; idx < size; ++idx) {
DataType dtype = reduces[idx]->dtype;
- res_handles[idx] = Var("reduce_temp" + std::to_string(idx), PointerType(PrimType(dtype)));
+ res_handles[idx] =
Review comment:
Removal of `AttrStmt` creation with `attr::storage_scope` will come after this PR, where all `attr::storage_scope` usages will be removed at once. A lot of places in the code base still assume the existence of `AttrStmt` with `attr::storage_scope`, so I don't want to remove some AttrStmt while leaving others in this PR.
--
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] tqchen commented on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-872276241
In this particular case, we might need to create a new buffer var and remap the use of the old buffer vars to new 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] tqchen commented on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-878485708
will let @vinx13 manage this PR
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r664941694
##########
File path: src/tir/ir/buffer.cc
##########
@@ -45,12 +45,27 @@ Array<PrimExpr> SimplifyArray(arith::Analyzer* ana, Array<PrimExpr> array) {
return array;
}
-Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, Span span) {
+Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, String storage_scope,
+ Span span) {
DataType storage_dtype = (dtype == DataType::Bool() ? DataType::Int(8) : dtype);
- return Buffer(Var(name, PointerType(PrimType(storage_dtype)), span), dtype, shape,
+ return Buffer(Var(name, PointerType(PrimType(storage_dtype), storage_scope), span), dtype, shape,
Array<PrimExpr>(), PrimExpr(), name, "", 0, 0, kDefault, span);
}
+String GetStorageScope(Var buffer_var) {
+ auto type = buffer_var->type_annotation;
+ const auto* ptr_type = type.as<PointerTypeNode>();
+ ICHECK(ptr_type) << "The provided variable is not of pointer type";
+ return ptr_type->storage_scope;
+}
+
+Var UpdateStorageScope(Var buffer_var, String storage_scope) {
Review comment:
Done
--
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] csullivan commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r668312348
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,91 @@
+/*
Review comment:
Thanks @masahi. I tracked this issue down to the buffer storage scope being set to `global` and removing the scope tag. Previously this was fine as the AttrStmtNode realize_scope annotation was used during texture lowering.
This demonstrate the value in removing the buffer storage scope in favor of consolidating around the PointerType's storage_scope. I look forward to that PR! Thanks again for 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] tqchen commented on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-871400179
Thanks @masahi . seems there are still testcases that we need to fix
--
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 removed a comment on pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi removed a comment on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-870508479
The CI job was killed during build twice for unclear reason.
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-875965614
> Should we remove `scope` from `Buffer`? The scope is now available from the type annotation of the `data` member in `BufferNode`.
Yes this is one of my follow-up items. Apparently `scope` in `Buffer` is only used by TensorIR related code.
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-870508479
The CI job was killed during build twice for unclear reason.
--
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] vinx13 commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r667369887
##########
File path: src/tir/transforms/lower_thread_allreduce.cc
##########
@@ -33,10 +33,33 @@
#include "../../runtime/thread_storage_scope.h"
#include "ir_utils.h"
+#include "update_pointer_storage_scope.h"
namespace tvm {
namespace tir {
+class UpdatePointerStorageScopeAllReduce final : public UpdatePointerStorageScope {
Review comment:
Can we merge this pass with `UpdatePointerStorageScope` by adding check for `attr::volatile_scope` in `VisitStmt_(const AttrStmtNode*)`
--
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] tqchen commented on a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r664543379
##########
File path: src/tir/ir/buffer.cc
##########
@@ -45,12 +45,27 @@ Array<PrimExpr> SimplifyArray(arith::Analyzer* ana, Array<PrimExpr> array) {
return array;
}
-Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, Span span) {
+Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, String storage_scope,
+ Span span) {
DataType storage_dtype = (dtype == DataType::Bool() ? DataType::Int(8) : dtype);
- return Buffer(Var(name, PointerType(PrimType(storage_dtype)), span), dtype, shape,
+ return Buffer(Var(name, PointerType(PrimType(storage_dtype), storage_scope), span), dtype, shape,
Array<PrimExpr>(), PrimExpr(), name, "", 0, 0, kDefault, span);
}
+String GetStorageScope(Var buffer_var) {
Review comment:
Consider to the caller side since directly exposing the function is more informative.
GetPtrStorageScope
##########
File path: src/tir/ir/buffer.cc
##########
@@ -45,12 +45,27 @@ Array<PrimExpr> SimplifyArray(arith::Analyzer* ana, Array<PrimExpr> array) {
return array;
}
-Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, Span span) {
+Buffer decl_buffer(Array<PrimExpr> shape, DataType dtype, String name, String storage_scope,
+ Span span) {
DataType storage_dtype = (dtype == DataType::Bool() ? DataType::Int(8) : dtype);
- return Buffer(Var(name, PointerType(PrimType(storage_dtype)), span), dtype, shape,
+ return Buffer(Var(name, PointerType(PrimType(storage_dtype), storage_scope), span), dtype, shape,
Array<PrimExpr>(), PrimExpr(), name, "", 0, 0, kDefault, span);
}
+String GetStorageScope(Var buffer_var) {
+ auto type = buffer_var->type_annotation;
+ const auto* ptr_type = type.as<PointerTypeNode>();
+ ICHECK(ptr_type) << "The provided variable is not of pointer type";
+ return ptr_type->storage_scope;
+}
+
+Var UpdateStorageScope(Var buffer_var, String storage_scope) {
Review comment:
Move this to the caller side since this is a quick convenient function. Consider rename to WithStorageScope
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -498,11 +498,12 @@ void CodeGenLLVM::AddAliasInfo(llvm::Instruction* inst, const VarNode* buffer, P
void CodeGenLLVM::GetAlignment(DataType t, const VarNode* buf_var, const PrimExpr& index,
int* p_alignment, int* p_native_bits) {
int max_align_bits = t.bits();
- auto it = alloc_storage_info_.find(buf_var);
- if (it != alloc_storage_info_.end()) {
- const StorageInfo& info = it->second;
Review comment:
Let us keep the original data structure info for now, in case we need to add more information later
##########
File path: Jenkinsfile
##########
@@ -282,7 +282,6 @@ stage('Unit Test') {
timeout(time: max_time, unit: 'MINUTES') {
sh "${docker_run} ${ci_arm} ./tests/scripts/task_ci_setup.sh"
sh "${docker_run} ${ci_arm} ./tests/scripts/task_python_unittest.sh"
- sh "${docker_run} ${ci_arm} ./tests/scripts/task_python_arm_compute_library.sh"
Review comment:
should not be part of the PR
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r665847958
##########
File path: src/tir/transforms/update_pointer_storage_scope.h
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file update_pointer_storage_scope.h
+ * \brief A pass to update storage scopes for buffer variables.
+ */
+#ifndef TVM_TIR_TRANSFORMS_UPDATE_POINTER_STORAGE_SCOPE_H_
+#define TVM_TIR_TRANSFORMS_UPDATE_POINTER_STORAGE_SCOPE_H_
+
+#include <tvm/tir/expr.h>
+#include <tvm/tir/op.h>
+#include <tvm/tir/stmt_functor.h>
+
+#include <unordered_map>
+
+namespace tvm {
+namespace tir {
+
+class UpdatePointerStorageScope : public StmtExprMutator {
Review comment:
This is used by passes that require modifying storage scopes after buffer creation. Currently used by `lower_thread_allreduce.cc` and `lower_warp_memory.cc`.
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-875332727
CI blocked by https://github.com/apache/tvm/pull/8400
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r667584153
##########
File path: src/tir/transforms/lower_thread_allreduce.cc
##########
@@ -33,10 +33,33 @@
#include "../../runtime/thread_storage_scope.h"
#include "ir_utils.h"
+#include "update_pointer_storage_scope.h"
namespace tvm {
namespace tir {
+class UpdatePointerStorageScopeAllReduce final : public UpdatePointerStorageScope {
Review comment:
This `attr::volatile_scope` is created inside this `AllocateNode` visitor. A visitor for `AttrStmtNode` don't get to see this.
--
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] vinx13 merged pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #8366:
URL: https://github.com/apache/tvm/pull/8366
--
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 #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#issuecomment-876101677
All tests passed, ready for review.
--
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 a change in pull request #8366: [Refactor] Enforce attaching storage scope to PointerType
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8366:
URL: https://github.com/apache/tvm/pull/8366#discussion_r668267879
##########
File path: src/tir/transforms/update_pointer_storage_scope.cc
##########
@@ -0,0 +1,91 @@
+/*
Review comment:
Hmm interesting. `attr::storage_scope` and the PointerType should have the same storage scope, by the invariant at https://github.com/apache/tvm/blob/3e5e0efe2dfde96f9b3a162ff66524ff79b62723/src/tir/ir/stmt.cc#L64-L73.
A possible explanation is that the value of `scope.tag.length()` at https://github.com/apache/tvm/blob/main/src/tir/transforms/lower_device_storage_access_info.cc#L70 is different between `main` and this branch. In this case, `GetMemoryInfo` assumes that a packed function `tvm.info.mem.global.texture` is defined, but I don't find it anywhere.
Do you have a repro script?
--
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