[IR][Relax] Improve highlighting in assert_structural_equal #16756
Conversation
|
For comparison, the error messages produced by Before: After: |
include/tvm/relax/expr.h
Outdated
|
|
||
| bool SEqualReduce(const VarBindingNode* other, SEqualReducer equal) const { | ||
| return equal.DefEqual(var, other->var) && equal(value, other->value); | ||
| return equal(value, other->value) && equal.DefEqual(var, other->var); |
There was a problem hiding this comment.
this might cause issue for self recursive definitions where the value can refer to the var. We should cross check the current handling of lambda expressions with self recursive reference, cc @slyubomirsky
There was a problem hiding this comment.
Good point. Are there any cases other than function definitions where the value can contain references to the bound variable? I think the highlighting of the RHS of a let binding is worth having a special case for lambda functions, but wouldn't want to have too many special cases.
There was a problem hiding this comment.
this is the only case that i am aware of
There was a problem hiding this comment.
Sounds good. I've added a unit test to ensure that recursively-defined lambda functions can be correctly compared. This unit test passes on main, failed on the first implementation as you pointed out, and now passes again with the handling of lambda functions.
|
Fixed the failing unit tests. Turns out, |
Prior to this commit, `tvm.ir.assert_structural_equal` would highlight an entire `relax::BindingBlock` if the number of elements in the binding block differs. This can result in the entire Relax function being highlighted, making it difficult to identify the location of the mismatch. This commit makes the following changes, to improve the error messages that occur when `tvm.ir.assert_structural_equal` raises an exception. - In `"node.StructuralEqual"`, set `defer_fails = true` when `assert_mode` is true. This highlights the first mismatch of an `Array<relax::Binding>`, rather than the entire array, in cases where the LHS and RHS have different sizes. - In the `SHashReduce` for `VarBinding` and `MatchCast`, visit the value first, and then the variable to which it is bound. This highlights the mismatched expression, rather than mismatches in the resulting struct info. - In `SEqualHandlerDefault::Impl::SEqualReduce`, defer the failure if enabled. This highlights the first mismatch, which may also have been deferred, rather than an early return a later mismatch occurs involving `NullOpt`.
Lunderberg
force-pushed
the
ir_improve_highlighting_assert_structural_equal
branch
from
da4500c to
0050cbc
Compare
slyubomirsky
left a comment
slyubomirsky
left a comment
There was a problem hiding this comment.
Definitely good to have an improvement to ergonomics (it is particularly unhelpful to have a gigantic underlined section). My apologies for missing the earlier discussion on recursive functions; the conclusion is correct and it looks to be handled correctly now. The only change that might be warranted would be a test case of a recursive function that does not match.
| // Recursive function definitions may reference the bound variable | ||
| // within the value being bound. In these cases, the | ||
| // `DefEqual(var, other->var)` must occur first, to ensure it is | ||
| // defined at point of use. | ||
| return equal.DefEqual(var, other->var) && equal.DefEqual(struct_info, other->struct_info) && |
There was a problem hiding this comment.
Per the above discussion, this sounds correct and indeed, local functions are the only time a local var can be used recursively in Relax.
Good call, and I'll add one in a follow-up PR. The failure mode that occurred for the recursive functions was an error being thrown for use of an undefined variable, which only required any comparison at all and is tested in |
A follow-up PR to apache#16756, adding an explicit unit test for `tvm.ir.assert_structural_equal` of two distinct recursive functions.
|
And the additional unit test is added in #16796. |
A follow-up PR to #16756, adding an explicit unit test for `tvm.ir.assert_structural_equal` of two distinct recursive functions.
…6756) * [IR][Relax] Improve highlighting in assert_structural_equal Prior to this commit, `tvm.ir.assert_structural_equal` would highlight an entire `relax::BindingBlock` if the number of elements in the binding block differs. This can result in the entire Relax function being highlighted, making it difficult to identify the location of the mismatch. This commit makes the following changes, to improve the error messages that occur when `tvm.ir.assert_structural_equal` raises an exception. - In `"node.StructuralEqual"`, set `defer_fails = true` when `assert_mode` is true. This highlights the first mismatch of an `Array<relax::Binding>`, rather than the entire array, in cases where the LHS and RHS have different sizes. - In the `SHashReduce` for `VarBinding` and `MatchCast`, visit the value first, and then the variable to which it is bound. This highlights the mismatched expression, rather than mismatches in the resulting struct info. - In `SEqualHandlerDefault::Impl::SEqualReduce`, defer the failure if enabled. This highlights the first mismatch, which may also have been deferred, rather than an early return a later mismatch occurs involving `NullOpt`. * DeferFail should follow assert_mode * Handle recursively defined lambda functions
…16796) A follow-up PR to apache#16756, adding an explicit unit test for `tvm.ir.assert_structural_equal` of two distinct recursive functions.
Prior to this commit, the `relax::analysis::CollectVarUsage` utility treated a local function definition as in-scope after visiting the body of the local function. As a result, recursive calls from a local function were incorrectly identified as calls to an undefined variable. This commit updates the `CollectVarUsage` to treat a local function definition as in-scope when inspecting the function body. This change is similar to the change made for structural equality in apache#16756.
* [Relax][Analysis] Handle recursive functions in CollectVarUsage Prior to this commit, the `relax::analysis::CollectVarUsage` utility treated a local function definition as in-scope after visiting the body of the local function. As a result, recursive calls from a local function were incorrectly identified as calls to an undefined variable. This commit updates the `CollectVarUsage` to treat a local function definition as in-scope when inspecting the function body. This change is similar to the change made for structural equality in #16756. * lint fixes
3 participants
Prior to this commit,
tvm.ir.assert_structural_equalwould highlight an entirerelax::BindingBlockif the number of elements in the binding block differs. This can result in the entire Relax function being highlighted, making it difficult to identify the location of the mismatch.This commit makes the following changes, to improve the error messages that occur when
tvm.ir.assert_structural_equalraises an exception.In
"node.StructuralEqual", setdefer_fails = truewhenassert_modeis true. This highlights the first mismatch of anArray<relax::Binding>, rather than the entire array, in cases where the LHS and RHS have different sizes.In the
SHashReduceforVarBindingandMatchCast, visit the value first, and then the variable to which it is bound. This highlights the mismatched expression, rather than mismatches in the resulting struct info.In
SEqualHandlerDefault::Impl::SEqualReduce, defer the failure if enabled. This highlights the first mismatch, which may also have been deferred, rather than an early return a later mismatch occurs involvingNullOpt.