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

Modify insert-gpu-allocs pass to also consider xegpu.store/load #1

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

dchigarev
Copy link

@dchigarev dchigarev commented Jul 31, 2024

Hi @Menooker, these changes in IMEX are required for linalg->xegpu->gpu exe pipeline in graph compiler.

The PR modifies insert-gpu-allocs that copies data between CPU and GPU so that it would also consider xegpu.load/store operations as a load/store operations and trigger the copy between devices.

I've tested the changes locally on the GPU pipeline (via xegpu).

Signed-off-by: dchigarev <dmitry.chigarev@intel.com>
@Menooker
Copy link
Owner

Menooker commented Aug 1, 2024

The patch LGTM. I am happy with merging this to our development branch. However, this looks like a bug fix which is worth upstreaming. I would suggest to submit it to the upstream IMEX repo. @kurapov-peter What do you think?

@dchigarev
Copy link
Author

dchigarev commented Aug 1, 2024

The patch LGTM. I am happy with merging this to our development branch. However, this looks like a bug fix which is worth upstreaming. I would suggest to submit it to the upstream IMEX repo. @kurapov-peter What do you think?

Yes, I also wanted to push this fix to IMEX, but in my understanding the time from PR submission until it's merged would be longer in case of the main IMEX branch (comparing with our dev branch). So I though I'll push this fix to our dev first and then upstream to IMEX in order to save time. Do you think I should do this the other way around (first to main IMEX and the port it to our dev branch)?

@Menooker
Copy link
Owner

Menooker commented Aug 1, 2024

The patch LGTM. I am happy with merging this to our development branch. However, this looks like a bug fix which is worth upstreaming. I would suggest to submit it to the upstream IMEX repo. @kurapov-peter What do you think?

Yes, I also wanted to push this fix to IMEX, but in my understanding the time from PR submission until it's merged would be longer in case of the main IMEX branch (comparing with our dev branch). So I though I'll push this fix to our dev first and then upstream to IMEX in order to save time. Do you think I should do this the other way around (first to main IMEX and the port it to our dev branch)?

Sure we can merge it to the development branch. I would suggest to apply the PR to a new branch "dev", which I have created in this repo just now. I have submitted this branch NewLower to IMEX upstream and there is a PR in that repo to track NewLower branch. We'd better not touch this branch and move future development on the new "dev" branch.

@dchigarev dchigarev changed the base branch from NewLower to dev August 1, 2024 09:12
@Menooker Menooker merged commit d4feb2b into Menooker:dev Aug 1, 2024
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