Skip to content
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

[Relax] Validate StructInfo of variable bindings #17332

Merged

Conversation

Lunderberg
Copy link
Contributor

In Relax, both the variable and the expression in a VarBinding may contain StructInfo annotations. Prior to this commit, these StructInfo annotations could be inconsistent, assigning an expression to a variable of incompatible type.

This commit updates the Relax well-formed checker to verify that the StructInfo of Relax variables accurately describes their contents.

@Lunderberg
Copy link
Contributor Author

This validation exposed a bug in StructInfoLCA, which only inspected the datatype of PrimStructInfo and not the known value. This bug was introduced in #15577, which added tracking fo known PrimStructInfo, but didn't update StructInfoLCA. This bug would result in incorrect known value annotations in the StructInfo, after a conditional statement. Since these annotations are later used to simplify known expressions, this bug could have resulted in incorrect simplifications.

The bug in StructInfoLCA is fixed as part of this PR.

In Relax, both the variable and the expression in a `VarBinding` may
contain `StructInfo` annotations.  Prior to this commit, these
`StructInfo` annotations could be inconsistent, assigning an
expression to a variable of incompatible type.

This commit updates the Relax well-formed checker to verify that the
`StructInfo` of Relax variables accurately describes their contents.
The `StructInfoLCA` determines the lowest common ancestor between two
`StructInfo` annotations.  This is primarily used in Relax to
determine the appropriate `StructInfo` annotation for a `relax::If`
node, given the `StructInfo` of each branch.  Prior to this commit,
when determining the LCA of two `PrimStructInfo` annotations, the
`StructInfoLCA` function only inspected the datatype of
`PrimStructInfo` annotations, and did not check for known values.  For
example, the LCA of `R.Prim(value=T.int64(128))` and
`R.Prim(value=T.int64(64))` is `R.Prim("int64")`, but was incorrectly
determined as `R.Prim(value=T.int64(128))` by the `StructInfoLCA`
function.

This commit updates `StructInfoLCA` to inspect the known values of a
`PrimStructInfo`, as well as the datatype.
@Lunderberg Lunderberg force-pushed the relax_well_formed_sinfo_of_var_binding branch from 8b7d373 to c4b980b Compare September 10, 2024 13:49
@Lunderberg
Copy link
Contributor Author

Rebased onto main to resolve conflicts in test_analysis_well_formed.py.

@tqchen
Copy link
Member

tqchen commented Sep 10, 2024

cc @yongwww can you help to take a look

Copy link
Member

@yongwww yongwww left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Lunderberg Lunderberg merged commit 72b75fe into apache:main Sep 11, 2024
18 of 19 checks passed
@Lunderberg Lunderberg deleted the relax_well_formed_sinfo_of_var_binding branch September 11, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants