-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] RemoveUnusedOutputs give unexpected results #17247
Comments
Prior to this commit, `NaN` values did not have any special handling in either `StructuralEqual` or `StructuralHash`. `StructuralEqual` checked whether the LHS and RHS were within some tolerance of each other. If the LHS and RHS are both `NaN`, this would evaluate to false. The updated `StructuralEqual` now checks for this case, and returns true if both sides are `NaN`. `StructuralHash` used the bit-pattern of a floating-point number to compute the hash. A `NaN` value may have any non-zero value in its mantissa, and so this could produce distinct hashes for ASTs that differ only by the choice of non-zero value. The updated `StructuralHash` uses the same `std::numeric_limits<double::quiet_NaN()` value for all `NaN` values. With these changes, `StructuralEqual` and `StructuralHash` can now compare two IR functions, even if they contain `NaN`. Closes apache#17247
Looks like the test case can be made even simpler, and isn't limited to @T.prim_func(private=True)
def func_1():
return T.float32("nan")
@T.prim_func(private=True)
def func_2():
return T.float32("nan")
tvm.ir.assert_structural_equal(func_1, func_2) I've implemented #17249 which should fix this issue, by having |
@Lunderberg Fixing for fixing the wrong implementation about Output IRs after the RemoveUnusedOutputs
|
Ooh, I had missed that part, and thought there was a |
Prior to this commit, the `relax.transform.RemoveUnusedOutputs` pass only marked a tuple element as used if it occurred in a `TupleGetItem` node. This ignored use cases where a tuple is used as an aggregate object, such as returning a tuple from a function. This would collect incorrect results for a Relax function that calls a subroutine, receives a tuple as the return value of the subroutine, then returns that tuple. This commit updates `RemoveUnusedOutputs` to look for usage of a tuple object, not just for usage in `TupleGetItem`. Closes apache#17247
The introduction of The So, if a function called a subroutine that produces a tuple, then immediately returned that tuple, the usage would fail to be collected, and the tuple elements would be erroneously replaced with |
Hi all, The pass
RemoveUnusedOutputs
seems to give an unexpected optimized result. Due to the lack of detailed documentation about this API (e.g.,relax.transform.RemoveUnusedOutputs
), I cannot confirm if the optimization result is wrong.In addition, another bug is about the API
tvm.ir.assert_structural_equal
, for the totally same mod, this API judge the structure of them as unequal. It was triggered by IRs with the string "nan".Actual behavior
Steps to reproduce
cc @Lunderberg @junrushao
The text was updated successfully, but these errors were encountered: