Lunderberg commented on PR #15807:
URL: https://github.com/apache/tvm/pull/15807#issuecomment-1788849978
> Then b is an unused var (by your definition), but not an impure call, so
it will be removed, which breaks the program.
Thank you, and I can reproduce the error with the test case below.
```python
def test_binding_block_keep_pure_func_used_only_for_impure():
"""Remove unused dataflow bindings
Removal of unused bindings may not remove side effects. Unused
bindings whose value is an impure operation (e.g. `R.call_packed`)
may not be removed, nor may any of their inputs.
"""
@R.function(private=True)
def before(x: R.Tensor((32, 32), "int32")):
y = x * R.const(2)
z = R.call_packed(
"function_maybe_with_side_effects", y, sinfo_args=(R.Tensor((32,
32), "int32"))
)
return R.tuple()
expected = before
after = remove_all_unused(before)
tvm.ir.assert_structural_equal(expected, after)
```
For the purpose of removing unused bindings, an argument to an impure
function is equivalent to an output, and should be retained. Thankfully, this
should be a pretty straightforward change to make.
> btw, since we have an assumption that binding block is impure, why do we
try to remove unused in binding block?
I'd phrase it slightly differently: A `BindingBlock` has no assumptions
about the purity of its bound values, where a `DataflowBlock` may assume that
all bound values are pure. That is, we don't need to assume that every step of
a `BindingBlock` is impure, but we also cannot assume that any step is pure.
Simplifications that require bound values to be pure are still legal within a
`BindingBlock`, so long as the purity is validated before making a change. The
advantage of the `DataflowBlock` is that it lets us skip that validation step,
because the contents are required to be pure.
In context of this PR, this check is encoded
[here](https://github.com/apache/tvm/pull/15807/files#diff-1f74c84f9a522574f3529c92e5eb683811e6c62c83412cf8575ff8e5b66167ecR249-R250).
A binding is removable if it is unused, and if it's computation is pure,
where purity can be established either from being within a `DataflowBlock`, or
from checking the purity explicitly. The bug is that the set of `unused_vars_`
is determined by flowing backwards from function outputs, where it should be
determined by flowing backwards from function outputs *and* arguments to impure
functions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
