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 2022/04/26 14:42:23 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #11129: [TIR] Reduced duplication in op.h
Lunderberg opened a new pull request, #11129:
URL: https://github.com/apache/tvm/pull/11129
Previously, `is_positive_int`, `is_negative_int`, `is_const_int`, and `as_const_int` had nearly duplicate type-checking logic. This allowed handling of Broadcast nodes to be diverge between the implementations. (e.g. `is_const_int(Broadcast(4,1), 4)` returns true, but `is_positive_int(Broadcast(4,1))` returns false.)
This PR changes `as_const_int` to contain the type-checking logic, including the handling of Broadcast nodes, with the other three functions implemented in terms of `as_const_int`.
--
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
[GitHub] [tvm] tqchen commented on pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
tqchen commented on PR #11129:
URL: https://github.com/apache/tvm/pull/11129#issuecomment-1109900766
Thanks @Lunderberg . The particular PR broadens the definition of `const_int` to include broadcasted values.
It might be useful to think about whether or not that would change the intended use of the original code that depends on those checks. To be extra careful, it would be helpful to introduce a separate function, `is_const_broadcast` to resolve the new intended checks.
Given it is the inline functions can be called frequently and the typec checking logics themselves are not longer compared to a call into a subfunction, we can keep implementation of is_positive_int as it is. I agree that in the case of broadcast pattern then the new impl would be better for a new function
--
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
[GitHub] [tvm] Lunderberg commented on pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11129:
URL: https://github.com/apache/tvm/pull/11129#issuecomment-1112525316
Rebase onto main to restart CI. Previous build failures seem to be unrelated issue on MacOS.
--
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
[GitHub] [tvm] Lunderberg commented on pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11129:
URL: https://github.com/apache/tvm/pull/11129#issuecomment-1109979956
Thank you, and I like the suggestion of making the broadcast handling more explicit. I was initially aiming for consistency and removing surprises when using similarly named APIs, and I hadn't been thinking of extracting the broadcast handling.
It looks like `is_const_int` was added #1789, and handled Broadcast nodes since its initial implementation. Glancing at the ~20 or so usages, I don't see anywhere that obviously needs handling. For now, I'm going to convert this to a draft, remove the Broadcast handling, and use any breakage to determine where there should be `is_const_int_broadcast` checks instead of `is_const_int`.
--
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
[GitHub] [tvm] tqchen commented on pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
tqchen commented on PR #11129:
URL: https://github.com/apache/tvm/pull/11129#issuecomment-1115183175
Thanks @Lunderberg !
--
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
[GitHub] [tvm] tqchen merged pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
tqchen merged PR #11129:
URL: https://github.com/apache/tvm/pull/11129
--
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
[GitHub] [tvm] Lunderberg commented on pull request #11129: [TIR] Reduced duplication in op.h
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #11129:
URL: https://github.com/apache/tvm/pull/11129#issuecomment-1114979104
I did another quick read at all existing cases of `is_const_int`, and didn't find any cases that rely on the Broadcast handling. Between that, the CI passing, and your description of the intent being to handle scalar types, this change looks reasonable to me.
--
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