Lunderberg commented on PR #16588: URL: https://github.com/apache/tvm/pull/16588#issuecomment-1955895172
> My main worry is we open a flood gate of introducing many recursive rewrite patterns to `ConstIntBound` itself. Ah, I think I see where I may have miscommunicated. There isn't actually any recursive rewriting being performed. The pattern matching are used to extract information, and not to perform any rewrites or allocations. Stating that `(A+B)*C < (A*B)*D` could be rearranged into `1/A + 1/B < D/C` was for the derivation of the tighter bounds, and isn't actually performed at runtime. I absolutely agree that `ConstIntBound` should be a pure analysis pass, and should not perform any rewrites to the argument. > Do you think if it is possible to introduce another subalayzer for this kind of prove given they are specific to LoRA, or have a stronger proof strength I think it could be moved to the `TryProve*` methods within the `RewriteSimplifier`, but hoisting it all the way out to a new subanalyzer probably wouldn't be feasible. It would need to simplify first, then check for the specific comparison being made, and then simplify again, which seems like a lot of overhead. My worry with introducing arguments for the proof strength is that it adds code complexity, and requires callers to know an additional argument. For cases that have a significant computational overhead, that's worth it to avoid surprising a user. For cases that don't, and would follow the same API, I'd want to avoid having that requirement where possible. Rather than enabling it by proof strength, what if we start with it it behind an extension flag ([link](https://github.com/apache/tvm/blob/main/include/tvm/arith/analyzer.h#L306)). Where more computationally expensive simplifications would always require a user to opt-in with a higher proof strength, individually-enabled flags could allow new simplifications to be introduced as opt-in, and only later be enabled by default. > In all cases, we should take a close look at the proves and make sure they are correct, since it is at center of what we are doing Absolutely agreed. For this case, I compared the range of output values for several `np.arange` test cases. Long-term, I wonder if it would be good to add fuzzing to the unit tests. As good as the unit tests are at the moment, I always like adding more steps that could catch an incorrect simplification. > If in the end we end up keep things in `ConstIntBound`, I think we should have a comment saying that this was really an exceptional case, and future changes to `ConstIntBound` should only be made in exceptional cases That makes sense, and I agree that it would be good to comment on 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. To unsubscribe, e-mail: commits-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
