[GitHub] [incubator-tvm] masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser
masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser URL: https://github.com/apache/incubator-tvm/pull/4497#issuecomment-583977652 @alexwong It seems you have problems with alexnet, vgg and mobilenet v2 on cuda. In my refactored version, I have no problem with these three. Have a look and try my script below. You can parse the module in two ways and compare the difference. https://github.com/masahi/torchscript-to-tvm/blob/master/torchvision_test.py#L51-L60 I guess the issue is in dtype or optional arguments handling in your op conversions. I've prepared [a branch](https://github.com/masahi/tvm/tree/torch-refactor) for the refactoring PR based on your current implementation, and I can reproduce errors on alexnet, vgg and mobilenet v2. The difference between this branch and the implementation at `torchscript-to-tvm` is mostly on op conversion map, that's why I think problems are there. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser
masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser URL: https://github.com/apache/incubator-tvm/pull/4497#issuecomment-583977652 @alexwong It seems you have problems with alexnet, vgg and mobilenet v2 on cuda. In my refactored version, I have no problem with these three. Have a look and try my script below. https://github.com/masahi/torchscript-to-tvm/blob/master/torchvision_test.py#L51-L60 I guess the issue is in dtype or optional arguments handling in your op conversions. I've prepared [a branch](https://github.com/masahi/tvm/tree/torch-refactor) for the refactoring PR based on your current implementation, and I can reproduce errors on alexnet, vgg and mobilenet v2. The difference between this branch and the implementation at `torchscript-to-tvm` is mostly on op conversion map, that's why I think problems are there. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser
masahi edited a comment on issue #4497: [Relay] Add a PyTorch to Relay Parser URL: https://github.com/apache/incubator-tvm/pull/4497#issuecomment-583977652 @alexwong It seems you have problems with alexnet, vgg and mobilenet v2 on cuda. In my refactored version, I have no problem with these three. Have a look and try my script below. https://github.com/masahi/torchscript-to-tvm/blob/master/torchvision_test.py#L51-L60 I guess the issue is in dtype or optional arguments handling in your op conversions. I've prepared [a branch](https://github.com/masahi/tvm/tree/torch-refactor) for the refactoring PR based on your current implementation, and I can reproduce errors on alexnet, vgg and mobilenet v2. The difference between this branch and my clean slate implementation at `torchscript-to-tvm` is mostly on op conversion map, that's why I think problems are there. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4497: [Relay] Add a PyTorch to Relay Parser
masahi commented on issue #4497: [Relay] Add a PyTorch to Relay Parser URL: https://github.com/apache/incubator-tvm/pull/4497#issuecomment-583977652 @alexwong It seems you have problems with alexnet, vgg and mobilenet v2 on cuda. In my refactored version, I have no problem with these three. Have a look and try my script below. https://github.com/masahi/torchscript-to-tvm/blob/master/torchvision_test.py#L51-L60 I guess the issue is in dtype or optional arguments handling in your op conversions. I've prepared [a branch](https://github.com/masahi/tvm/tree/torch-refactor) for the refactoring PR based on your current implementation, and I can reproduce errors on alexnet, vgg and mobilenet v2. The difference between this branch and my clean slate implementation at `torchscript-to-tvm` is mostly on op conversion map, that's why I think problems are there. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4497: [Relay] Add a PyTorch to Relay Parser
masahi commented on issue #4497: [Relay] Add a PyTorch to Relay Parser URL: https://github.com/apache/incubator-tvm/pull/4497#issuecomment-583959013 @alexwong need to rebase and ``` tvm.ndarray -> tvm.runtime.ndarray ``` 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request #4828: [QNN][TFLite] TFLite rounding mode support
FrozenGene commented on a change in pull request #4828: [QNN][TFLite] TFLite
rounding mode support
URL: https://github.com/apache/incubator-tvm/pull/4828#discussion_r376858216
##
File path: include/tvm/relay/qnn/attrs.h
##
@@ -44,14 +44,15 @@ struct RequantizeAttrs : public
tvm::AttrsNode {
.set_default(-1);
TVM_ATTR_FIELD(rounding).set_default("UPWARD")
.describe("Defines the rounding direction when the value is midway
between"
- "two representable values. There are two supported modes -
UPWARD"
- "or TONEAREST. Both modes behave exactly same except at the"
+ "two representable values. There are two 3 modes - UPWARD,
TONEAREST"
+ "or TFLITE. UP/TONEAREST modes behave exactly same except at
the"
"midpoints between the two representable values. At the
midpoint,"
"UPWARD rounds towards positive infinity (for example -1.5
will be"
"rounded to -1). TONEAREST is the standard rounding where
the"
"value is rounded away from zero at midpoints (for example,
-1.5"
"rounds to -2). More context can be found at following gblic
manual"
-
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;);
+
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;
+ "TFLITE mode is more complicated, referring to tflite
implementation.");
Review comment:
Yep, should remove the tflite code link. glibc is one website document html,
is no problem. But the tflite reference is the code link (no website document
html), we should remove it. Let us add more detail doc here: `TFLITE mode is
more complicated, referring to our tflite rounding implementation.` to instruct
reader to read our code implementation.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request #4828: [QNN][TFLite] TFLite rounding mode support
FrozenGene commented on a change in pull request #4828: [QNN][TFLite] TFLite
rounding mode support
URL: https://github.com/apache/incubator-tvm/pull/4828#discussion_r376858216
##
File path: include/tvm/relay/qnn/attrs.h
##
@@ -44,14 +44,15 @@ struct RequantizeAttrs : public
tvm::AttrsNode {
.set_default(-1);
TVM_ATTR_FIELD(rounding).set_default("UPWARD")
.describe("Defines the rounding direction when the value is midway
between"
- "two representable values. There are two supported modes -
UPWARD"
- "or TONEAREST. Both modes behave exactly same except at the"
+ "two representable values. There are two 3 modes - UPWARD,
TONEAREST"
+ "or TFLITE. UP/TONEAREST modes behave exactly same except at
the"
"midpoints between the two representable values. At the
midpoint,"
"UPWARD rounds towards positive infinity (for example -1.5
will be"
"rounded to -1). TONEAREST is the standard rounding where
the"
"value is rounded away from zero at midpoints (for example,
-1.5"
"rounds to -2). More context can be found at following gblic
manual"
-
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;);
+
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;
+ "TFLITE mode is more complicated, referring to tflite
implementation.");
Review comment:
Yep, should remove the link. glibc is one website document html, is no
problem. But the tflite reference is the code link (no website document html),
we should remove it. Let us add more detail doc here: `TFLITE mode is more
complicated, referring to our tflite rounding implementation.` to instruct
reader to read our code implementation.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] LiangHao151941 commented on issue #4828: [QNN][TFLite] TFLite rounding mode support
LiangHao151941 commented on issue #4828: [QNN][TFLite] TFLite rounding mode support URL: https://github.com/apache/incubator-tvm/pull/4828#issuecomment-583939949 > Let us break the model into layer by layer and compare with tflite. I want to describe my development way, maybe it could help you. For example, we have mobilenetv2 quantized model, you could get the quantized tensorflow and tflite model. Then you could call tflite_convert (feed it quantized tensorflow model) and set the output layer (for example, just the first convolution layer), then you get one quantized tflite model only contain the first convolution layer of mobilenet v2. After you verify it correctly, you could go on until you finish the whole model e2e correctly. Command example: tflite_convert --input_format=TENSORFLOW_GRAPHDEF --graph_def_file="xx.pb" --output_file=xx.tflite --output_format=TFLITE --input_arrays=input --input_shapes=1,224,224,3 --std_dev_values=127 --mean_values=127 --inference_type=QUANTIZED_UINT8 --inference_input_type=QUANTIZED_UINT8 --default_ranges_min=0 --default_ranges_max=6 --output_arrays=xx Thanks for the detailed instruction here. Will follow up. > I think when you verify, you could run on cpu firstly locally to find issue, then consider gpu ci issue. No problem. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite] TFLite rounding mode support
LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite]
TFLite rounding mode support
URL: https://github.com/apache/incubator-tvm/pull/4828#discussion_r376854118
##
File path: src/relay/qnn/util.cc
##
@@ -100,35 +134,54 @@ Expr FixedPointMultiply(Expr tensor, double multiplier,
const Array&
// (from the right, rightmost bit is bit 0). The computation is performed in
// higher precision to avoid overflow in multiplying two int32 values.
Expr scalar = MakeConstantScalar(hp_dtype, fixed_point_multiplier);
- tensor = Multiply(tensor, scalar);
+ Expr scaled_tensor = Multiply(tensor, scalar);
// 4) Find the rounding scalar. This depends on where the final decimal
// point sits. As we will be right shifting the multiplied_t, we need to
// first calculate the total_right_shift.
int total_right_shift = right_shift + 31;
int64_t pos_rounding_value = (1ll << (total_right_shift - 1));
+ auto nearest_rounding_scalar =
+[&](const Expr& input_tensor, int right_shift) -> Expr {
+ int64_t pos_rounding_value = (1ll << (right_shift - 1));
+ auto pos_rounder = MakeConstantScalar(hp_dtype, pos_rounding_value);
+ auto neg_rounder = MakeConstantScalar(hp_dtype, pos_rounding_value - 1);
+ auto pos_rounder_t = Full(pos_rounder, input_shape, hp_dtype);
+ auto neg_rounder_t = Full(neg_rounder, input_shape, hp_dtype);
+
+ auto zero_t = Zeros(input_shape, hp_dtype);
+ return Where(
+GreaterEqual(input_tensor, zero_t), pos_rounder_t, neg_rounder_t);
Review comment:
I'll add some comments for this lambda function, if it is the suggestion
here that I understand.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite] TFLite rounding mode support
LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite]
TFLite rounding mode support
URL: https://github.com/apache/incubator-tvm/pull/4828#discussion_r376853818
##
File path: src/relay/qnn/util.cc
##
@@ -22,13 +22,47 @@
* \brief Utility functions for QNN.
*/
+#include
#include "util.h"
#include "../pass/pattern_util.h"
namespace tvm {
namespace relay {
namespace qnn {
+/* \brief This function implements the rounding part of ARMv7 NEON VQRDMULH
+ * instruction. For code reuse, the multiplied tensor is directly passed in
+ * as parameter.
+ */
+Expr SaturatingRoundingDoublingHigh32(const Expr& input_tensor,
+ const Expr& multiplier_expr,
+ const Expr& scaled_tensor,
+ const Array& input_shape) {
+DataType hp_dtype = DataType::Int(64);
+int32_t pos_nudge_value = (1ll << 30);
+int32_t neg_nudge_value = 1 - (1ll << 30);
+auto pos_nudge = MakeConstantScalar(hp_dtype, pos_nudge_value);
+auto neg_nudge = MakeConstantScalar(hp_dtype, neg_nudge_value);
+auto pos_nudge_t = Full(pos_nudge, input_shape, hp_dtype);
+auto neg_nudge_t = Full(neg_nudge, input_shape, hp_dtype);
+
+auto int32_min = MakeConstantScalar(
+hp_dtype, std::numeric_limits::min());
+auto int32_max = MakeConstantScalar(
+hp_dtype, std::numeric_limits::max());
+auto int32_min_t = Full(int32_min, input_shape, hp_dtype);
+auto int32_max_t = Full(int32_max, input_shape, hp_dtype);
Review comment:
This patch is for a unique request that needs bit-exact execution compared
to tflite implementation(not only on arm targets). I believe moving this
generic implementation into relay help us to gain more control over compiler
behavior and reduce gaps between different targets and implementations.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite] TFLite rounding mode support
LiangHao151941 commented on a change in pull request #4828: [QNN][TFLite]
TFLite rounding mode support
URL: https://github.com/apache/incubator-tvm/pull/4828#discussion_r376852996
##
File path: include/tvm/relay/qnn/attrs.h
##
@@ -44,14 +44,15 @@ struct RequantizeAttrs : public
tvm::AttrsNode {
.set_default(-1);
TVM_ATTR_FIELD(rounding).set_default("UPWARD")
.describe("Defines the rounding direction when the value is midway
between"
- "two representable values. There are two supported modes -
UPWARD"
- "or TONEAREST. Both modes behave exactly same except at the"
+ "two representable values. There are two 3 modes - UPWARD,
TONEAREST"
+ "or TFLITE. UP/TONEAREST modes behave exactly same except at
the"
"midpoints between the two representable values. At the
midpoint,"
"UPWARD rounds towards positive infinity (for example -1.5
will be"
"rounded to -1). TONEAREST is the standard rounding where
the"
"value is rounded away from zero at midpoints (for example,
-1.5"
"rounds to -2). More context can be found at following gblic
manual"
-
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;);
+
"https://www.gnu.org/software/libc/manual/html_node/Rounding.html.;
+ "TFLITE mode is more complicated, referring to tflite
implementation.");
Review comment:
TFLite version is 1.13, conforming with relay tflite frontend. The rounding
implementation is here
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/lite/kernels/internal/common.h#L105,
but according to community norm, external links should be better removed to
avoid license issues? @FrozenGene
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] LiangHao151941 commented on issue #4828: [QNN][TFLite] TFLite rounding mode support
LiangHao151941 commented on issue #4828: [QNN][TFLite] TFLite rounding mode support URL: https://github.com/apache/incubator-tvm/pull/4828#issuecomment-583936335 > :( This will be a challenging task. I think, instead of trying to fix the full e2e test, we should now focus on unit tests. I can help with improving unit test coverage. That would be appreciated, thanks! > Do we run TFLIte tests on GPU? Little confused. Yes, according to jenkins report. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] tobegit3hub commented on issue #4459: [RUNTIME] Implement TVMDSOOp(TensorFlow custom op) for TVM runtime
tobegit3hub commented on issue #4459: [RUNTIME] Implement TVMDSOOp(TensorFlow custom op) for TVM runtime URL: https://github.com/apache/incubator-tvm/pull/4459#issuecomment-583923302 Thanks @zhiics . We are new to TVM and not sure we can add walk-thought test with this op since it is the "framework" of TensorFlow custom op but not the specified op. The unit tests of these functions require to mock TensorFlow runtime. It would be great if someone can help to guide the testing. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] inadob commented on issue #4840: [Frontend][TFlite] use qnn helper function in softmax
inadob commented on issue #4840: [Frontend][TFlite] use qnn helper function in softmax URL: https://github.com/apache/incubator-tvm/pull/4840#issuecomment-583880570 LGTM too 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] dpankratz opened a new pull request #4852: [Bugfix] Fixed crash when caused by reversing bitwise operators
dpankratz opened a new pull request #4852: [Bugfix] Fixed crash when caused by reversing bitwise operators URL: https://github.com/apache/incubator-tvm/pull/4852 I ran into a bug when using the bitwise operators bitwise_xor, bitwise_and, and bitwise_or. The following code results in a Python TypeError: ``` a = tvm.var() b = 10 ^ a ``` For reference this code is accepted: ``` a = tvm.var() b = a ^ 10 ``` Analogous crashes occur for the case of `10 | a` and `10 & a`. This pull requests fixes this crash for the operators bitwise_xor, bitwise_and, and bitwise_or. It also adds a regression test. cc @tqchen @inadob 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800855
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+if (other == get()->data) {
+ return 0;
+}
+return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+const auto* ptr = get();
+if (ptr == nullptr) {
+ return 0;
+}
+return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's
rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
Review comment:
Use copy constructor directly in a constructor.
```
explicit FromStd(std::string other)
: data_container(other)
```
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.
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801581
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
Review comment:
consider put data before size
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800653
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+if (other == get()->data) {
+ return 0;
+}
+return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+const auto* ptr = get();
+if (ptr == nullptr) {
+ return 0;
+}
+return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's
rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other)
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802418
##
File path: tests/cpp/container_test.cc
##
@@ -226,6 +226,94 @@ TEST(Map, Iterator) {
CHECK(map2[a].as()->value == 2);
}
+TEST(String, MoveFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s(std::move(source));
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), 0);
+}
+
+TEST(String, CopyFromStd) {
+ using namespace std;
+ string source = "this is a string";
+ string expect = source;
+ String s{source};
+ string copy = (string)s;
+ CHECK_EQ(copy, expect);
+ CHECK_EQ(source.size(), expect.size());
+}
+
+TEST(String, Assignment) {
+ using namespace std;
+ String s{string{"hello"}};
+ s = string{"world"};
+ CHECK_EQ(s == "world", true);
+ string s2{"world2"};
+ s = std::move(s2);
+ CHECK_EQ(s == "world2", true);
+}
+
+TEST(String, Comparisons) {
+ using namespace std;
+ string source = "a string";
+ string mismatch = "a string but longer";
+ String s{source};
+
+ CHECK_EQ(s == source, true);
+ CHECK_EQ(s == mismatch, false);
+ CHECK_EQ(s == source.data(), true);
+ CHECK_EQ(s == mismatch.data(), false);
+
+ // Check '\0' handling
+ string v1 = "hello world";
Review comment:
Add compare string with different length
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802267
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
Review comment:
Using this->compare to add more test coverage
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801571
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
Review comment:
use uint64_t here.
Rationale: we will likely need to store big binary blob that goes beyond 32
bits. Because of the memory alignment, uint32_t won't save us more memory here
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800691
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+if (other == get()->data) {
+ return 0;
+}
+return std::memcmp(get()->data, other, len);
+ }
+
+ /*!
+ * \brief Returns a pointer to the char array in the string.
+ *
+ * \return const char*
+ */
+ const char* c_str() const { return get()->data; }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t size() const {
+const auto* ptr = get();
+if (ptr == nullptr) {
+ return 0;
+}
+return ptr->size;
+ }
+
+ /*!
+ * \brief Return the length of the string
+ *
+ * \return size_t string length
+ */
+ size_t length() const { return size(); }
+
+ /*!
+ * \brief Return the data pointer
+ *
+ * \return const char* data pointer
+ */
+ const char* data() const { return get()->data; }
+
+ /*!
+ * \brief Convert String to an std::sting object
+ *
+ * \return std::string
+ */
+ operator std::string() const { return std::string{get()->data, size()}; }
+
+ TVM_DEFINE_OBJECT_REF_METHODS(String, ObjectRef, StringObj);
+
+ private:
+ /*! \return the internal StringObj pointer */
+ const StringObj* get() const { return operator->(); }
+};
+
+/*! \brief An object representing string moved from std::string. */
+class StringObj::FromStd : public StringObj {
+ public:
+ /*!
+ * \brief Construct a new FromStd object
+ *
+ * \param other The moved/copied std::string object
+ *
+ * \note If user passes const reference, it will trigger copy. If it's
rvalue,
+ * it will be moved into other.
+ */
+ explicit FromStd(std::string other) { data_container.swap(other); }
+
+ private:
+ /*! \brief Container that holds the memory. */
+ std::string data_container;
+
+ friend class String;
+};
+
+inline String::String(std::string other) {
+ auto ptr = make_object(std::move(other));
+ ptr->size = ptr->data_container.size();
+ ptr->data = ptr->data_container.data();
+ data_ = std::move(ptr);
+}
+
+inline String String::operator=(std::string other)
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801684
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
Review comment:
document all the parameters via \param
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376801874
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
Review comment:
Let us implement operator==(and redirect !=) to be consistent with
std::string version.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String
container
URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376802203
##
File path: include/tvm/runtime/container.h
##
@@ -274,7 +298,246 @@ class ADT : public ObjectRef {
TVM_DEFINE_OBJECT_REF_METHODS(ADT, ObjectRef, ADTObj);
};
+/*! \brief An object representing string. It's POD type. */
+class StringObj : public Object {
+ public:
+ /*! \brief The length of the string object. */
+ uint32_t size;
+
+ /*! \brief The pointer to string data. */
+ const char* data;
+
+ static constexpr const uint32_t _type_index = TypeIndex::kDynamic;
+ static constexpr const char* _type_key = "runtime.String";
+ TVM_DECLARE_FINAL_OBJECT_INFO(StringObj, Object);
+
+ private:
+ /*! \brief String object which is moved from std::string container. */
+ class FromStd;
+
+ friend class String;
+};
+
+/*!
+ * \brief Reference to string objects.
+ *
+ * \code
+ *
+ * // Example to create runtime String reference object from std::string
+ * std::string s = "hello world";
+ *
+ * // You can create the reference from existing std::string
+ * String ref{std::move(s)};
+ *
+ * // You can rebind the reference to another string.
+ * ref = std::string{"hello world2"};
+ *
+ * // You can use the reference as hash map key
+ * std::unordereddata, size()) == 0;
+ }
+
+ /*!
+ * \brief Compare is not equal to other std::string
+ *
+ * \param other The other string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const std::string& other) const { return !operator==(other);
}
+
+ /*!
+ * \brief Compare is equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator==(const char* other) const { return !operator!=(other); }
+
+ /*!
+ * \brief Compare is not equal to other char string
+ *
+ * \param other The other char string
+ *
+ * \return the comparison result
+ */
+ bool operator!=(const char* other) const {
+return size() != std::strlen(other) || compare(other, size()) != 0;
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const String& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const std::string& other, size_t len) const {
+return compare(other.data(), len);
+ }
+
+ /*!
+ * \brief Compares this to other for at most len chars
+ *
+ * \return zero if both char sequences compare equal. negative if this appear
+ * before other, positive otherwise.
+ */
+ int compare(const char* other, size_t len) const {
+if (other == get()->data) {
+ return 0;
+}
+return std::memcmp(get()->data, other, len);
Review comment:
Sorry, my previous comment was not correct, as memcmp did not support the
case where the size of two strings did not equal each other. We will have to
implement a internal memncmp using for loop(which have size of two strings
here)
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String container URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800596 ## File path: include/tvm/runtime/container.h ## @@ -24,11 +24,35 @@ #ifndef TVM_RUNTIME_CONTAINER_H_ #define TVM_RUNTIME_CONTAINER_H_ +// Reference for feature test macros of string_view: +// https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations +// https://en.cppreference.com/w/User:D41D8CD98F/feature_testing_macros +#if defined(__cpp_lib_experimental_string_view) && \ +__cpp_lib_experimental_string_view >= 201411 +#define TVM_USE_CXX14_STRING_VIEW 1 Review comment: chang to TVM_USE_CXX14_STING_VIEW_HASH 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4628: [Object] Add String container
tqchen commented on a change in pull request #4628: [Object] Add String container URL: https://github.com/apache/incubator-tvm/pull/4628#discussion_r376800566 ## File path: include/tvm/runtime/container.h ## @@ -24,11 +24,35 @@ #ifndef TVM_RUNTIME_CONTAINER_H_ #define TVM_RUNTIME_CONTAINER_H_ +// Reference for feature test macros of string_view: +// https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations Review comment: Let us comment about the rationale here about why do we include std::experimental::string_view in c++14. And comment that the usage is only limited to hash 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #4771: [Relay] Added Merge Composite pass
comaniac commented on a change in pull request #4771: [Relay] Added Merge Composite pass URL: https://github.com/apache/incubator-tvm/pull/4771#discussion_r376800880 ## File path: python/tvm/relay/transform.py ## @@ -513,6 +513,31 @@ def Legalize(legalize_map_attr_name="FTVMLegalize"): return _transform.Legalize(legalize_map_attr_name) +def MergeComposite(pattern_table): +"""Merge multiple operators into a single composite relay function. + +Parameters +-- +pattern_table : list(tuple) +A list of (pattern_name, pattern) tuples. Review comment: Because the patterns have priorities and the index of this list implies the order. There was a discussion about how to deal with multiple patterns match to the same sub graph, and this is the workaround for 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4628: [Object] Add String container
tqchen edited a comment on issue #4628: [Object] Add String container URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583871162 @FrozenGene In general I agree that we should avoid std::experimental. In this particular case, i think the usage is fair, because it is guarded by marco tests and is only under a very limited case we a std::hash function that can hash a string without copying it(instead of using the string_view data structure). - T0: We could have implemented a hash function by ourselves, but the hash itself may be inconsistent with the std version. - T1: While the std::experimental::string_view's hash could have been inconsistent with the std::string version as per compiler version(because of experimental), in practice it is consistent with std::string as per string_view proposal(and can be confirmed using different compilers). More importantly, it is also fine if the hash is inconsistent with the std ver(then we will be the case of T1. Given the above consideration, I think it is fine to permit the limited usecase. However, I agree that we should have a more careful documentation about the std::experimental use case here and only limit it to the specific usecase. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4628: [Object] Add String container
tqchen commented on issue #4628: [Object] Add String container URL: https://github.com/apache/incubator-tvm/pull/4628#issuecomment-583871162 @FrozenGene In general I agree that we should avoid std::experimental. In this particular case, i think the usage is fair, because it is guarded by marco tests and is only under a very limited case we a std::hash function that can hash a string without copying it. - T0: We could have implemented a hash function by ourselves, but the hash itself may be inconsistent with the std version. - T1: While the std::experimental::string_view's hash could have been inconsistent with the std::string version as per compiler version(because of experimental), in practice it is consistent with std::string as per string_view proposal(and can be confirmed using different compilers). More importantly, it is also fine if the hash is inconsistent with the std ver(then we will be the case of T1. Given the above consideration, I think it is fine to permit the limited usecase. However, I agree that we should have a more careful documentation about the std::experimental use case here and only limit it to the specific usecase. 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: us...@infra.apache.org With regards, Apache Git Services
[incubator-tvm] branch master updated: [CI][DOCKER] Update ci-lint to pylint2.4.4 (#4851)
This is an automated email from the ASF dual-hosted git repository. tqchen pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-tvm.git The following commit(s) were added to refs/heads/master by this push: new 13f2155 [CI][DOCKER] Update ci-lint to pylint2.4.4 (#4851) 13f2155 is described below commit 13f2155e8f89eb7b6a47b54bd565087dc538cdf2 Author: Tianqi Chen AuthorDate: Sun Feb 9 09:12:19 2020 -0800 [CI][DOCKER] Update ci-lint to pylint2.4.4 (#4851) --- docker/Dockerfile.ci_lint | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/Dockerfile.ci_lint b/docker/Dockerfile.ci_lint index b049a71..1c72ee7 100644 --- a/docker/Dockerfile.ci_lint +++ b/docker/Dockerfile.ci_lint @@ -17,7 +17,7 @@ # For lint test # CI docker lint env -# tag: v0.51 +# tag: v0.60 FROM ubuntu:16.04 RUN apt-get update && apt-get install -y sudo wget @@ -25,7 +25,7 @@ COPY install/ubuntu_install_python.sh /install/ubuntu_install_python.sh RUN bash /install/ubuntu_install_python.sh RUN apt-get install -y doxygen graphviz git -RUN pip3 install cpplint pylint==1.9.4 mypy +RUN pip3 install cpplint pylint==2.4.4 mypy # java deps for rat COPY install/ubuntu_install_java.sh /install/ubuntu_install_java.sh
[GitHub] [incubator-tvm] tqchen merged pull request #4851: [CI][DOCKER] Update ci-lint to pylint2.4.4
tqchen merged pull request #4851: [CI][DOCKER] Update ci-lint to pylint2.4.4 URL: https://github.com/apache/incubator-tvm/pull/4851 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on issue #4748: [RELAY] Support RelayBuild with Only Constants
FrozenGene commented on issue #4748: [RELAY] Support RelayBuild with Only Constants URL: https://github.com/apache/incubator-tvm/issues/4748#issuecomment-583851947 >I even think it is just okay to leave it as it is because it means we just return a null pointer when returning an empty module, which is a valid return. Considering mod is is essentially a shared pointer, the failure/crash in dereferencing a null pointer sounds good to me. IMO, I agree with @zhiics 's point here. >There is one more question though: what the target of the dummy function should be? I currently set it to llvm. Should be `target_host_`. `llvm` just means x86 cpu. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] uday60 edited a comment on issue #4830: libnnvm_compiler.so file missing | Commands inside
uday60 edited a comment on issue #4830: libnnvm_compiler.so file missing | Commands inside URL: https://github.com/apache/incubator-tvm/issues/4830#issuecomment-583401837 @tqchen I have been using an earlier version of TVM. Now things have changed the following script is giving me a segmentation fault. I have already raised the issue here but no replies. https://discuss.tvm.ai/t/conversion-script-terminate-called-after-throwing-an-instance-of-std-bad-alloc/5601 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] optima2005 commented on issue #4009: [RFC] 3D model support
optima2005 commented on issue #4009: [RFC] 3D model support URL: https://github.com/apache/incubator-tvm/issues/4009#issuecomment-583818527 @masahi Sorry, I have been busy at some other things recently. I'd like to be back if I could have more free time again. Thanks! 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: us...@infra.apache.org With regards, Apache Git Services
