Lunderberg closed pull request #15916: [Unity][Transform] Handle relax.Var as
call_tir args when lowering
URL: https://github.com/apache/tvm/pull/15916
--
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
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1802355290
Closing this PR, as it is superseded by #16067 and #16068.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1792924890
@slyubomirsky @tqchen Can you take a look at PRs
https://github.com/apache/tvm/pull/16067 and
https://github.com/apache/tvm/pull/16068? The former introduces new
functionality through
tqchen commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1788006758
> as many constraints exposed to the C++ type system as possible
This is indeed also a tradeoff here in terms of IR design. We cannot encode
all constraints, for example the ANF
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1787961361
> The simpler approach, is actually to just enable well-form check to detect
the related issues and require tuple to be inlined.
Oh, certainly agreed that the well-formed check is
tqchen commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1786224271
The simpler approach, is actually to just enable well-form check to detect
the related issues and require tuple to be inlined.
Normalizer serves as a way to create new bound
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1786122024
> Are we presently using inline tuples to simplify things significantly?
I haven't found any locations that would require anything more than a
binding lookup. Of the 19 instances
slyubomirsky commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1785967855
Great work in making the full comparison list, Eric. Are we presently using
inline tuples to simplify things significantly? I really don't think it's that
big a deal to check the type
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1785503708
I think the major issue is the inconsistency of normalization. From the
code itself, there is no clear expectation of this being the normal form for
Relax, and the majority of the
tqchen commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1783840165
What we are discussing is closely related to our original consideration of
what forms a normal form and the general needs. Let me elaborate a bit more
The purpose of normal form is to
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1783586454
> My concerns were addressed, but we should have agreement on the design
question @tqchen posed before we merge (if we will merge).
Oh, absolutely. The comment was to ensure that
slyubomirsky commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1782267071
If we intend to have special cases like `call_tir` where one argument _must_
be a tuple literal (i.e., not following the normal rule of the type system that
any member of the type
tqchen commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1781923886
The principle here is we make common cases(and their optimizations easy),
while placing burdens on less common cases.
As for the pass writing patterns, most of our current relax
slyubomirsky commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1781537269
Making a new IR node for `call_tir` would entail a lot of revisions, but it
might be worth it since it is very important to the language. If that's
something we would consider, we
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1781380637
@slyubomirsky I pulled in the infer struct info improvements and the unit
test updates from PR#15971, so this should now include all fixes from both sets
of changes.
--
This is an
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1781157045
This does increase the complexity of passes that interact with `call_tir`,
but it also decreases the complexity of all other passes. If we require an
in-line tuple for `call_tir`, then
Lunderberg commented on code in PR #15916:
URL: https://github.com/apache/tvm/pull/15916#discussion_r1373155497
##
python/tvm/relax/op/base.py:
##
@@ -97,7 +97,11 @@ def call_tir(
ret: Call
A call node for the call_tir operator.
"""
-if isinstance(args,
slyubomirsky commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1780155887
See also [this
issue](https://discuss.tvm.apache.org/t/validity-of-common-subexpression-elimination-pass-that-simplifies-call-tir-args/15832).
Allowing `call_tir` to be more general
tqchen commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1780084913
Thanks for the PR, I know this is indeed a generalization and there are some
tradeoffs to be considered here. Specifically, we should consider the following
alternative:
- C0: We
tqchen commented on code in PR #15916:
URL: https://github.com/apache/tvm/pull/15916#discussion_r1372345395
##
include/tvm/relax/expr_functor.h:
##
@@ -278,6 +278,37 @@ class ExprVisitor : public ExprFunctor {
virtual void VisitSpan(const Span& span);
virtual void
Lunderberg commented on code in PR #15916:
URL: https://github.com/apache/tvm/pull/15916#discussion_r1372341781
##
src/relax/transform/call_tir_rewrite.cc:
##
@@ -111,41 +111,69 @@ class CallTIRMutator : public ExprMutator {
<< expr->struct_info_;
}
slyubomirsky commented on code in PR #15916:
URL: https://github.com/apache/tvm/pull/15916#discussion_r1372276835
##
python/tvm/relax/op/base.py:
##
@@ -97,7 +97,11 @@ def call_tir(
ret: Call
A call node for the call_tir operator.
"""
-if isinstance(args,
slyubomirsky commented on code in PR #15916:
URL: https://github.com/apache/tvm/pull/15916#discussion_r1372273991
##
src/relax/transform/call_tir_rewrite.cc:
##
@@ -111,41 +111,69 @@ class CallTIRMutator : public ExprMutator {
<< expr->struct_info_;
}
slyubomirsky commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1779975180
I think you may need to update the StructInfo inference for
`call_tir_inplace` like in #15971, since (without modification) that assumes
the argument is a tuple literal. The test
Lunderberg commented on PR #15916:
URL: https://github.com/apache/tvm/pull/15916#issuecomment-1764836058
Rebased onto `unity` head to ensure the CI tests haven't broken in the
meantime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on
Lunderberg opened a new pull request, #15916:
URL: https://github.com/apache/tvm/pull/15916
Prior to this commit, several transforms assumed that the arguments passed
to a `call_tir` builtin were provided as in-line `relax::Tuple` objects.
Because it would be equally valid for the
26 matches
Mail list logo
