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

[Bugfix][TIR] Handle AttrStmt of upcoming tir.Var in ConvertSSA #16682

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

Lunderberg
Copy link
Contributor

In some cases, an AttrStmt may legally refer to a TIR variable that hasn't yet been defined. For example, the
"pragma_parallel_launch_point" attribute, which annotates a variable that is about to occur in a ForNode. Prior to this commit, ConvertSSA treated the AttrStmt as the usage of a variable, followed by a nested definition to be de-duplicated. This resulted in the output AttrStmt containing a reference to an undefined variable.

This commit updates ConvertSSA to handle this case. If an AttrStmt refers to a not-yet-defined variable, the body is visited before marking it as defined.

This implementation may be simplified in the future by moving "pragma_parallel_launch_point" to be an annotation on the ForNode, rather than an AttrStmt.

In some cases, an `AttrStmt` may legally refer to a TIR variable that
hasn't yet been defined.  For example, the
`"pragma_parallel_launch_point"` attribute, which annotates a variable
that is about to occur in a ForNode.  Prior to this commit,
`ConvertSSA` treated the `AttrStmt` as the usage of a variable,
followed by a nested definition to be de-duplicated.  This resulted in
the output `AttrStmt` containing a reference to an undefined variable.

This commit updates `ConvertSSA` to handle this case.  If an
`AttrStmt` refers to a not-yet-defined variable, the body is visited
before marking it as defined.

This implementation may be simplified in the future by
moving "pragma_parallel_launch_point" to be an annotation
on the `ForNode`, rather than an `AttrStmt`.
@Lunderberg Lunderberg force-pushed the bugfix_tir_ssa_pragma_parallel branch from 794e51c to f480fa4 Compare March 6, 2024 14:58
@Lunderberg
Copy link
Contributor Author

@slyubomirsky This should unblock #16682, as it resolves the issue found in tests/python/tir-transform/test_tir_transform_hoist_if.py::test_hoisting_block_scope_4.

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.

Thank you for the change. Would this also work for attributes that do act as points of definition?

@Lunderberg
Copy link
Contributor Author

Would this also work for attributes that do act as points of definition?

It should, yes. This behavior is tested in TestThreadIdxReusedWithinAndAcrossFunctions, where the "thread_extent" attribute does act as a point of definition.

@tqchen tqchen merged commit 898f87f into apache:main Mar 9, 2024
19 checks passed
@Lunderberg Lunderberg deleted the bugfix_tir_ssa_pragma_parallel branch March 10, 2024 17:20
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Mar 12, 2024
…he#16682)

In some cases, an `AttrStmt` may legally refer to a TIR variable that
hasn't yet been defined.  For example, the
`"pragma_parallel_launch_point"` attribute, which annotates a variable
that is about to occur in a ForNode.  Prior to this commit,
`ConvertSSA` treated the `AttrStmt` as the usage of a variable,
followed by a nested definition to be de-duplicated.  This resulted in
the output `AttrStmt` containing a reference to an undefined variable.

This commit updates `ConvertSSA` to handle this case.  If an
`AttrStmt` refers to a not-yet-defined variable, the body is visited
before marking it as defined.

This implementation may be simplified in the future by
moving "pragma_parallel_launch_point" to be an annotation
on the `ForNode`, rather than an `AttrStmt`.
thaisacs pushed a commit to thaisacs/tvm that referenced this pull request Apr 3, 2024
…he#16682)

In some cases, an `AttrStmt` may legally refer to a TIR variable that
hasn't yet been defined.  For example, the
`"pragma_parallel_launch_point"` attribute, which annotates a variable
that is about to occur in a ForNode.  Prior to this commit,
`ConvertSSA` treated the `AttrStmt` as the usage of a variable,
followed by a nested definition to be de-duplicated.  This resulted in
the output `AttrStmt` containing a reference to an undefined variable.

This commit updates `ConvertSSA` to handle this case.  If an
`AttrStmt` refers to a not-yet-defined variable, the body is visited
before marking it as defined.

This implementation may be simplified in the future by
moving "pragma_parallel_launch_point" to be an annotation
on the `ForNode`, rather than an `AttrStmt`.
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