Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Prior to this commit, symbolic variables inferred from the parameters were retained in the output function's ret_struct_info. This is ill-formed, as the parameters from which these symbolic variables are inferred are no longer part of the function signature.

This commit updates LazyGetInput to use EraseToWellDefined to remove any symbolic variables from ret_struct_info that cannot be inferred from the remaining arguments.

Prior to this commit, symbolic variables inferred from the parameters
were retained in the output function's `ret_struct_info`.  This is
ill-formed, as the parameters from which these symbolic variables are
inferred are no longer part of the function signature.

This commit updates `LazyGetInput` to use `EraseToWellDefined` to
remove any symbolic variables from `ret_struct_info` that cannot be
inferred from the remaining arguments.
@Lunderberg
Copy link
Contributor Author

@slyubomirsky This is the follow-up to #16798, which I ended up making much sooner than I had initially planned.

Copy link
Contributor

@slyubomirsky slyubomirsky left a comment

Choose a reason for hiding this comment

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

Good catch; I didn't realize that we were losing shape vars from the signature

@Lunderberg
Copy link
Contributor Author

Good catch; I didn't realize that we were losing shape vars from the signature

Thank you. This ended up resulting from some debugging I did with tvm.relax.ir.instrument.WellFormedInstrument to validate the IRModule before and after every transform. In working my way to the actual bug, I ran into a number of cases where the IRModule was ill-formed, but in a way that didn't impact the compiled output.

@Lunderberg Lunderberg merged commit 6f74762 into apache:main Apr 4, 2024
@Lunderberg Lunderberg deleted the lazy_get_input_well_formed_ret_struct_info branch April 4, 2024 02:37
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.

2 participants