You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "kparzysz-quic (via GitHub)" <gi...@apache.org> on 2023/10/04 13:19:19 UTC

[PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

kparzysz-quic opened a new pull request, #15872:
URL: https://github.com/apache/tvm/pull/15872

   The values of `min` and `extent` in Range should have the same type, but the usage of convenience may often produce type mismatches, for example `Range(0, dim)` will assume int32 for the `min`. These mismatches can then cause hard-to-track errors, in particular assertions in the arithmetic simplifier.
   This is already done for `For` statement, but not in other places.
   
   This patch introduces `DataType::WidestOf` function, that returns the widest type from the given list. It is then used to promote the integer bounds if such widest type exists.


-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "kparzysz-quic (via GitHub)" <gi...@apache.org>.
kparzysz-quic commented on PR #15872:
URL: https://github.com/apache/tvm/pull/15872#issuecomment-1747814460

   I'm not sure what you think.  This is adding a lot of code for a simple thing, but I'm hoping that the extra functions may be useful in other cases as well.


-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "kparzysz-quic (via GitHub)" <gi...@apache.org>.
kparzysz-quic commented on code in PR #15872:
URL: https://github.com/apache/tvm/pull/15872#discussion_r1346140580


##########
include/tvm/runtime/data_type.h:
##########
@@ -237,6 +241,49 @@ class DataType {
     }
   }
 
+  /*!
+   * \brief Return the widest type from a given array.
+   *
+   * Given an array of types, return the one of those types that can
+   * hold all values representable in the other types, or "Void" if
+   * such type is not present in the array.
+   */
+  static DataType WidestOf(const std::vector<DataType>& types) {

Review Comment:
   Would `std::initializer_list` be ok?
   
   Edit: Nevermind, I'll do the template.



-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #15872:
URL: https://github.com/apache/tvm/pull/15872#issuecomment-1747015636

   In most cases the intention was to construct IRs that have consistent types. So insert casting like `cast(i32_val, i64)` is still not desirable for cases like arith simplifier.
   
   How about we do the following things:
   - Auto promote trivial constants, in which case no cast will be introduced
   - Error out(instead of silently promote) in constructor for cases that are in-consistent, so we can have callers to fix the type tracking when possible.
   


-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on code in PR #15872:
URL: https://github.com/apache/tvm/pull/15872#discussion_r1346082809


##########
include/tvm/runtime/data_type.h:
##########
@@ -237,6 +241,49 @@ class DataType {
     }
   }
 
+  /*!
+   * \brief Return the widest type from a given array.
+   *
+   * Given an array of types, return the one of those types that can
+   * hold all values representable in the other types, or "Void" if
+   * such type is not present in the array.
+   */
+  static DataType WidestOf(const std::vector<DataType>& types) {

Review Comment:
   is it possible to use template trick or overloading to handle two/three case, mainly avoid the vector passing



-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "kparzysz-quic (via GitHub)" <gi...@apache.org>.
kparzysz-quic closed pull request #15872: [TIR] Autocorrect Range min and extent types when possible
URL: https://github.com/apache/tvm/pull/15872


-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "kparzysz-quic (via GitHub)" <gi...@apache.org>.
kparzysz-quic commented on code in PR #15872:
URL: https://github.com/apache/tvm/pull/15872#discussion_r1346140580


##########
include/tvm/runtime/data_type.h:
##########
@@ -237,6 +241,49 @@ class DataType {
     }
   }
 
+  /*!
+   * \brief Return the widest type from a given array.
+   *
+   * Given an array of types, return the one of those types that can
+   * hold all values representable in the other types, or "Void" if
+   * such type is not present in the array.
+   */
+  static DataType WidestOf(const std::vector<DataType>& types) {

Review Comment:
   Would `std::initializer_list` be ok?



-- 
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

Re: [PR] [TIR] Autocorrect Range min and extent types when possible [tvm]

Posted by "kparzysz-quic (via GitHub)" <gi...@apache.org>.
kparzysz-quic commented on PR #15872:
URL: https://github.com/apache/tvm/pull/15872#issuecomment-1755669269

   This cannot be done correctly in `Range` alone.  This is because there are cases like `Bind(var, Range(a, b))`.  If `a` and `b` are both immediates of different types, then there is no way to infer the correct common type, because the correct type is that of the `var`, which is not known to the range.


-- 
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