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 2020/04/08 06:04:50 UTC

[GitHub] [incubator-tvm] zhiics opened a new pull request #5276: [REFACTOR][IR] Move to runtime::String

zhiics opened a new pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276
 
 
   #5250 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics edited a comment on issue #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on issue #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#issuecomment-610771891
 
 
   @tqchen One problem of replacing the default container StringImm with runtime String is that some of the IR nodes are using PrimExpr which could be both StringImm or IntImm (they are implicitly converted from std::string, integers, or floats). For example,
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/expr.h#L707
   
   and 
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/stmt.h#L147-L149
   
   This would fail if we pass string and default it to runtime String, i.e.
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/python/tvm/tir/expr.py#L977
   
   Should we explicitly convert it from runtime String to StringImm in this case either before the constructor in Python or in the PackedFunc in the C++? This looks pretty ugly. Could you suggest some better solutions?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405879639
 
 

 ##########
 File path: src/node/serialization.cc
 ##########
 @@ -102,7 +102,7 @@ class NodeIndexer : public AttrVisitor {
       for (const auto& kv : n->data) {
         MakeIndex(const_cast<Object*>(kv.second.get()));
       }
-    } else {
+    } else if (!node->IsInstance<StringObj>()) {
 
 Review comment:
   @tqchen Could you please take a look at the serialization of String here? I am not sure why the size of the String I loaded back is always 0.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405893661
 
 

 ##########
 File path: src/node/serialization.cc
 ##########
 @@ -102,7 +102,7 @@ class NodeIndexer : public AttrVisitor {
       for (const auto& kv : n->data) {
         MakeIndex(const_cast<Object*>(kv.second.get()));
       }
-    } else {
+    } else if (!node->IsInstance<StringObj>()) {
 
 Review comment:
   Indeed, we should design a special serialization mechanism for String. A quick idea is to just print out the string content, we can also think about base64 encoding.  I can try to take a stab tomorrow. 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405879639
 
 

 ##########
 File path: src/node/serialization.cc
 ##########
 @@ -102,7 +102,7 @@ class NodeIndexer : public AttrVisitor {
       for (const auto& kv : n->data) {
         MakeIndex(const_cast<Object*>(kv.second.get()));
       }
-    } else {
+    } else if (!node->IsInstance<StringObj>()) {
 
 Review comment:
   @tqchen Could you please take a look at the serialization of String here? I am not sure why the size of the String I loaded back is always 0. Should we save it out separately like DLTensor?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405851011
 
 

 ##########
 File path: src/relay/backend/graph_runtime_codegen.cc
 ##########
 @@ -419,7 +419,7 @@ class GraphRuntimeCodegen
     auto pf1 = GetPackedFunc("relay.backend._CompileEngineLower");
     Target target;
     // Handle external function
-    if (func->GetAttr<tir::StringImm>(attr::kCompiler).defined()) {
+    if (func->GetAttr<runtime::String>(attr::kCompiler).defined()) {
 
 Review comment:
   Let us consider expose runtime::String to the tvm namespace as well(by put using runtime::String in the tvm namespace of container.h) So that we can directly type GetAttr<String>

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405851288
 
 

 ##########
 File path: src/relay/transforms/device_annotation.cc
 ##########
 @@ -574,7 +574,7 @@ Pass RewriteAnnotatedOps(int fallback_device) {
     return Downcast<Function>(relay::RewriteAnnotatedOps(f, fallback_device));
   };
   return CreateFunctionPass(pass_func, 1, "RewriteAnnotatedOps",
-                            {tir::StringImmNode::make("InferType")});
+                            {runtime::String("InferType")});
 
 Review comment:
   I wonder if we can directly do {"InferType"} here now(if String's auto conversion works)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405852469
 
 

 ##########
 File path: src/relay/transforms/device_annotation.cc
 ##########
 @@ -574,7 +574,7 @@ Pass RewriteAnnotatedOps(int fallback_device) {
     return Downcast<Function>(relay::RewriteAnnotatedOps(f, fallback_device));
   };
   return CreateFunctionPass(pass_func, 1, "RewriteAnnotatedOps",
-                            {tir::StringImmNode::make("InferType")});
+                            {runtime::String("InferType")});
 
 Review comment:
   yeah, the reason seems because Array<runtime::String/stringimm> doesn't really take initializers like {"aa", "bb"}

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405893378
 
 

 ##########
 File path: src/relay/transforms/device_annotation.cc
 ##########
 @@ -574,7 +574,7 @@ Pass RewriteAnnotatedOps(int fallback_device) {
     return Downcast<Function>(relay::RewriteAnnotatedOps(f, fallback_device));
   };
   return CreateFunctionPass(pass_func, 1, "RewriteAnnotatedOps",
-                            {tir::StringImmNode::make("InferType")});
+                            {runtime::String("InferType")});
 
 Review comment:
   If you remove the explicit keyword of String's constructor from std::string, or const char* it might work

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#issuecomment-612060782
 
 
   Thanks @zhiics !

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405879639
 
 

 ##########
 File path: src/node/serialization.cc
 ##########
 @@ -102,7 +102,7 @@ class NodeIndexer : public AttrVisitor {
       for (const auto& kv : n->data) {
         MakeIndex(const_cast<Object*>(kv.second.get()));
       }
-    } else {
+    } else if (!node->IsInstance<StringObj>()) {
 
 Review comment:
   @tqchen Could you please please take a look at the serialization of String here? I am not sure why the size of the String I loaded back is always 0.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405850603
 
 

 ##########
 File path: include/tvm/ir/expr.h
 ##########
 @@ -107,11 +107,12 @@ class PrimExpr : public BaseExpr {
    * \param value The value to be constructed.
    */
   TVM_DLL PrimExpr(float value);  // NOLINT(*)
+
   /*!
-   * \brief construct from string.
-   * \param str The value to be constructed.
+   * \brief construct from runtime String.
+   * \param value The value to be constructed.
    */
-  TVM_DLL PrimExpr(std::string str);  // NOLINT(*)
+  TVM_DLL PrimExpr(runtime::String value);  // NOLINT(*)
 
 Review comment:
   Can we simply remove this constructor and explicily construct StringImm in the places that we need them

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405954029
 
 

 ##########
 File path: include/tvm/ir/expr.h
 ##########
 @@ -107,11 +107,12 @@ class PrimExpr : public BaseExpr {
    * \param value The value to be constructed.
    */
   TVM_DLL PrimExpr(float value);  // NOLINT(*)
+
   /*!
-   * \brief construct from string.
-   * \param str The value to be constructed.
+   * \brief construct from runtime String.
+   * \param value The value to be constructed.
    */
-  TVM_DLL PrimExpr(std::string str);  // NOLINT(*)
+  TVM_DLL PrimExpr(runtime::String value);  // NOLINT(*)
 
 Review comment:
   Considering the number of files changed so far, I'd like to keep this for now. It seems a bit more work. I can send a separate PR to handle it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405599551
 
 

 ##########
 File path: python/tvm/runtime/_ffi_container_api.py
 ##########
 @@ -0,0 +1,20 @@
+# 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.
+"""FFI APIs for tvm.runtime.container"""
+import tvm._ffi
+
+tvm._ffi._init_api("runtime.container", __name__)
 
 Review comment:
   We can just put it under runtime api given there is no additional container namespace 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405958799
 
 

 ##########
 File path: src/node/serialization.cc
 ##########
 @@ -102,7 +102,7 @@ class NodeIndexer : public AttrVisitor {
       for (const auto& kv : n->data) {
         MakeIndex(const_cast<Object*>(kv.second.get()));
       }
-    } else {
+    } else if (!node->IsInstance<StringObj>()) {
 
 Review comment:
   I saved it separately to handle this for now 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on issue #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#issuecomment-610771891
 
 
   @tqchen One problem of replacing the default container StringImm with runtime String is that some of the IR nodes are using PrimExpr which could be both StringImm or IntImm (they are implicitly converted from std::string, integers, or floats). For example,
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/expr.h#L707
   
   This would fail if we pass string and default it to runtime String, i.e.
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/python/tvm/tir/expr.py#L977
   
   Should we explicitly convert it from runtime String to StringImm in this case either before the constructor in Python or in the PackedFunc in the C++? This looks pretty ugly. Could you suggest some better solutions?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics edited a comment on issue #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on issue #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#issuecomment-610771891
 
 
   @tqchen One problem of replacing the default container StringImm with runtime String is that some of the IR nodes are using PrimExpr which could be both StringImm or IntImm (they are implicitly converted from std::string, integers, or floats). For example,
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/expr.h#L707
   
   and 
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/stmt.h#L147-L149
   This would fail if we pass string and default it to runtime String, i.e.
   
   https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/python/tvm/tir/expr.py#L977
   
   Should we explicitly convert it from runtime String to StringImm in this case either before the constructor in Python or in the PackedFunc in the C++? This looks pretty ugly. Could you suggest some better solutions?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405950942
 
 

 ##########
 File path: src/relay/transforms/device_annotation.cc
 ##########
 @@ -574,7 +574,7 @@ Pass RewriteAnnotatedOps(int fallback_device) {
     return Downcast<Function>(relay::RewriteAnnotatedOps(f, fallback_device));
   };
   return CreateFunctionPass(pass_func, 1, "RewriteAnnotatedOps",
-                            {tir::StringImmNode::make("InferType")});
+                            {runtime::String("InferType")});
 
 Review comment:
   I tried a bit more. It looks we actually need the constructor from `const char*` in order to make the initializer_list work. Previously, we only have the constructor from `std::string`. I will add it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen merged pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#issuecomment-611015061
 
 
   In the case of IRnodes that need StringImm, I think we can do explicit conversation for now 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405852469
 
 

 ##########
 File path: src/relay/transforms/device_annotation.cc
 ##########
 @@ -574,7 +574,7 @@ Pass RewriteAnnotatedOps(int fallback_device) {
     return Downcast<Function>(relay::RewriteAnnotatedOps(f, fallback_device));
   };
   return CreateFunctionPass(pass_func, 1, "RewriteAnnotatedOps",
-                            {tir::StringImmNode::make("InferType")});
+                            {runtime::String("InferType")});
 
 Review comment:
   yeah, the reason seems because `Array<runtime::String/stringimm>` doesn't really take initializers like {"aa", "bb"}

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405851767
 
 

 ##########
 File path: include/tvm/ir/expr.h
 ##########
 @@ -107,11 +107,12 @@ class PrimExpr : public BaseExpr {
    * \param value The value to be constructed.
    */
   TVM_DLL PrimExpr(float value);  // NOLINT(*)
+
   /*!
-   * \brief construct from string.
-   * \param str The value to be constructed.
+   * \brief construct from runtime String.
+   * \param value The value to be constructed.
    */
-  TVM_DLL PrimExpr(std::string str);  // NOLINT(*)
+  TVM_DLL PrimExpr(runtime::String value);  // NOLINT(*)
 
 Review comment:
   Yeah, I consider that as well, but it seems there are too many places using PrimExpr. Need sometime to see which one may take string. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5276: [REFACTOR][IR] Move to runtime::String
URL: https://github.com/apache/incubator-tvm/pull/5276#discussion_r405954029
 
 

 ##########
 File path: include/tvm/ir/expr.h
 ##########
 @@ -107,11 +107,12 @@ class PrimExpr : public BaseExpr {
    * \param value The value to be constructed.
    */
   TVM_DLL PrimExpr(float value);  // NOLINT(*)
+
   /*!
-   * \brief construct from string.
-   * \param str The value to be constructed.
+   * \brief construct from runtime String.
+   * \param value The value to be constructed.
    */
-  TVM_DLL PrimExpr(std::string str);  // NOLINT(*)
+  TVM_DLL PrimExpr(runtime::String value);  // NOLINT(*)
 
 Review comment:
   Considering the number of files changed so file, I'd like to keep this for now. It seems a bit more work. I can send a separate PR to handle it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services