Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/relax/transform/canonicalize_bindings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}

if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
!visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
// Case 4a: Var = DataflowVar, but used outside this DataflowBlock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Case 4a: Var = DataflowVar, but used outside this DataflowBlock
// Case 4a: Var = DataflowVar, but not used outside this DataflowBlock

It appears the text was copied from case 4b. I also think it would be clearer to write out that it is the var that is not used outside the DF block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, and updated the comment to be more explicit. I've also changed "this DataflowBlock" to "the DataflowBlock containing the binding", since this function is called after the entire function is visited, not during the visit of any specific dataflow block.

//
// For these three cases, the trivial binding can be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// For these three cases, the trivial binding can be
// For these four cases, the trivial binding can be

😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off by one errors, my two one nemesis! (Thank you, and fixed.)

// unwrapped, using the bound variable directly at the point
// of use.
plan.replace_usage.Set(bound_var->vid, bound_to);
plan.bindings_to_remove.insert(bound_var->vid);
} else {
// Case 4: Var = DataflowVar
// Case 4b: Var = DataflowVar, and used outside this DataflowBlock
//
// Replacing a Var with a DataflowVar could result in illegal
// use of a DataflowVar outside of a DataflowBlock. Instead,
Expand Down
34 changes: 34 additions & 0 deletions tests/python/relax/test_transform_canonicalize_bindings.py
Original file line number Diff line number Diff line change
Expand Up @@ -977,5 +977,39 @@ def main():
verify(TestChainAssignments, Expected)


def test_trivial_binding_of_replaced_non_dataflow_var():
@I.ir_module
class Before:
@R.function
def main(param_tuple: R.Tuple([R.Tensor])):
with R.dataflow():
A = param_tuple[0]
B = A
C = R.add(A, B)
R.output(A, B, C)
return C

@I.ir_module
class Expected:
@R.function
def main(param_tuple: R.Tuple([R.Tensor])):
with R.dataflow():
A = param_tuple[0]
C = R.add(A, A)
R.output(C)
return C

After = CanonicalizeBindings()(Before)
tvm.ir.assert_structural_equal(After, Expected)

def _get_binding_names(mod):
return [binding.var.name_hint for binding in mod["main"].body.blocks[0].bindings]

expected_names = _get_binding_names(Expected)
after_names = _get_binding_names(After)

assert after_names == expected_names


if __name__ == "__main__":
tvm.testing.main()