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

Add separate function to apply shared memory indexing #129

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

harsh-nod
Copy link
Contributor

This PR introduces a new function to apply shared
memory indexing corrections. Previously, this was being done as part of post expansion. With the new PR, we look at the entire graph holistically and apply indexing changes to reads from shared memory.

Copy link
Contributor

@raikonenfnu raikonenfnu left a comment

Choose a reason for hiding this comment

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

Overall looks good/ really nice cleanup! just 1 Q.

write.index = remove_global_indexing(
read.index, tilingConstraints
)
write.index = deepcopy(read.index)
Copy link
Contributor

@Hardcode84 Hardcode84 Sep 6, 2024

Choose a reason for hiding this comment

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

Why do we need deepcopy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if we just assign read.index to write.index, then when we modify write.index in the corrections pass it will also affect read.index. But did you have a better way to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

But remove_global_indexing is already creating a copy, it shouldn't affect original index object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. remove_global_indexing is creating a copy, but that copy will be assigned to write.index and read.index which is not what we want. If you remove the deepcopy, two of the lit tests fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I was able to figure this out. I have removed the deep copy. @Hardcode84 - can you take a look when you get a chance? thanks!

This PR introduces a new function to apply shared
memory indexing corrections. Previously, this was being done
as part of post expansion. With the new PR, we look at the
entire graph holistically and apply indexing changes to
reads from shared memory.

Signed-off-by: Harsh Menon <harsh@nod-labs.com>
Signed-off-by: Harsh Menon <harsh@nod-labs.com>
@harsh-nod harsh-nod force-pushed the indexing_fixes branch 2 times, most recently from 593cc79 to 64204c5 Compare September 9, 2024 20:58
This PR modifies the way the index is being updated
so that we allocate a new index whenever the index
is being set. Also, rather than overload the write index
to also return the register index, we add a separate
register_index property for Write nodes.

Signed-off-by: Harsh Menon <harsh@nod-labs.com>
Copy link
Contributor

@Hardcode84 Hardcode84 left a comment

Choose a reason for hiding this comment

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

LGTM!

@harsh-nod harsh-nod merged commit f27ae6f into iree-org:main Sep 9, 2024
6 checks passed
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