[GitHub] [tvm] Lunderberg commented on a diff in pull request #11646: [Arith] Simplification of ceil, log2, and left_shift

[GitBox <gi...@apache.org>]

Lunderberg commented on code in PR #11646:
URL: https://github.com/apache/tvm/pull/11646#discussion_r894624013


##########
src/arith/const_int_bound.cc:
##########
@@ -341,6 +343,18 @@ class ConstIntBoundAnalyzer::Impl
     }
   }
 
+  Entry VisitLeftShift(const CallNode* op) {
+    Entry a = VisitExpr(op->args[0]);
+    Entry b = VisitExpr(op->args[1]);
+
+    // Until C++20, performing a left shift is only well-defined for
+    // positive arguments.  If we have a negative argument, it just
+    // means we couldn't prove that the inputs were positive.
+    a.min_value = std::max(int64_t(0), a.min_value);
+    b.min_value = std::max(int64_t(0), b.min_value);
+    return BinaryOpBoundary(a, b, InfAwareLeftShift);

Review Comment:
   Hmm, this might depend on the target.  For the `CodegenC` backend, it doesn't narrow the bounds for any legal use of `<<`, because negative arguments are entirely undefined behavior anyways.  Taking the `max` here is a way to express that same constraint.  That is, even if we can't prove that the argument is non-negative, its use in a bitshifting operator provides a constraint.
   
   That said, since my primary goal is to improve simplifications from `ceil_log2`, perhaps it would be better to look for a `ceil(log2(x))` call, and use that directly.  The aggressive optimizations that C++ compilers make based on undefined behavior reasoning are controversial for a reason, and I'd like to avoid introducing similar logic in TVM unless required.



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