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

Cache ir_utils::allTvs as part of Fusion #2873

Merged
merged 8 commits into from
Sep 2, 2024
Merged

Cache ir_utils::allTvs as part of Fusion #2873

merged 8 commits into from
Sep 2, 2024

Conversation

jacobhinkle
Copy link
Collaborator

This caches ir_utils::allTvs(fusion) as fusion->allTvs(). The cache is automatically invalidated whenever the TV graph topology changes; this mechanism is the same one used to recompute Expr uses automatically.

This caches `ir_utils::allTvs(fusion)` as `fusion->allTvs()`. The cache
is automatically invalidated whenever the TV graph topology changes;
this mechanism is the same one used to recompute `Expr` uses
automatically.
@jacobhinkle
Copy link
Collaborator Author

!build

@csarofeen csarofeen marked this pull request as ready for review September 1, 2024 17:28
@csarofeen csarofeen requested a review from naoyam September 1, 2024 17:28
@csarofeen
Copy link
Collaborator

Topo sort test was unhappy because it uses an unregistered expression and this approach triggers a traversal that needs dispatch. trying again.

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

Do we still need to keep ir_utils::allTvs?

csrc/fusion.cpp Show resolved Hide resolved
csrc/fusion.h Outdated Show resolved Hide resolved
@naoyam
Copy link
Collaborator

naoyam commented Sep 1, 2024

!build

@csarofeen
Copy link
Collaborator

!build

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

LGTM

@naoyam
Copy link
Collaborator

naoyam commented Sep 1, 2024

!build

@csarofeen
Copy link
Collaborator

@naoyam let's hold off on merging this. I replaced all the uses of allTvs in the codebase and am now getting a segfault. There must still be something wrong with it.

@csarofeen
Copy link
Collaborator

Do we still need to keep ir_utils::allTvs?

I'll cleanup in a follow up.

@csarofeen
Copy link
Collaborator

!build

@csarofeen csarofeen merged commit 744bf54 into main Sep 2, 2024
36 checks passed
@csarofeen csarofeen deleted the cache_all_tvs branch September 2, 2024 13:17
naoyam pushed a commit that referenced this pull request Sep 3, 2024
Follow up to #2873 removes all uses
of the ir_utils variant in favor of the Fusion variant.

---------

Co-authored-by: Jacob Hinkle <[email protected]>
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