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

Tweak synchronous memcpy implementation #1809

Merged
merged 29 commits into from
Feb 18, 2020
Merged

Conversation

AlexVlx
Copy link
Contributor

@AlexVlx AlexVlx commented Jan 23, 2020

The existing one can have issues on certain systems, therefore this limits use of direct memcpy via largeBAR to sizes where it is unequivocally better.

Also addresses SWDEV-220030.

@mangupta mangupta added this to the rocm-3.1 milestone Jan 24, 2020
@jeffdaily
Copy link
Contributor

FYI jenkins build 15 failed and 17 failed, but build 16 succeeded. I believe this PR is actually passing CI now if we could try kicking off another try.

@gargrahul
Copy link
Contributor

gargrahul commented Jan 31, 2020

FYI jenkins build 15 failed and 17 failed, but build 16 succeeded. I believe this PR is actually passing CI now if we could try kicking off another try.

Done. build 18 is in progress now.

@jeffdaily
Copy link
Contributor

@gargrahul did something change recently? I'm seeing this error in the CI logs:
Error: docker: Error response from daemon: Unknown runtime specified nvidia.

Comment on lines 207 to 211
if (da.handle == 0 || sa.handle == 0) { // Handle degenerate case.
throwing_result_check(hsa_memory_copy(dst, src, n),
__FILE__, __func__, __LINE__);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we still need this change as we have observed hangs in hsa_memory_copy().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore the comment missed your latest commit :)

@jedwards-AMD
Copy link
Contributor

Lets merge this in.

Copy link
Contributor

@jedwards-AMD jedwards-AMD left a comment

Choose a reason for hiding this comment

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

Lets merge this.

@mangupta mangupta added the pr:resolve_conflicts PR requires additional testing label Feb 11, 2020
@mangupta mangupta modified the milestones: rocm-3.1, rocm-3.2 Feb 12, 2020
@mangupta mangupta added pr:needs_updates PR initially approved. But needs rework and removed pr:resolve_conflicts PR requires additional testing labels Feb 18, 2020
Copy link
Contributor

@mangupta mangupta left a comment

Choose a reason for hiding this comment

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

Approved based on test build report 736

@mangupta mangupta merged commit 9b4f39e into master Feb 18, 2020
@mangupta mangupta deleted the feature_memcpy_the_2nd branch February 18, 2020 15:20
mangupta pushed a commit that referenced this pull request Feb 27, 2020
Fixes SWDEV-223910 and SWDEV-223663
AryanSalmanpour pushed a commit to AryanSalmanpour/HIP that referenced this pull request Feb 28, 2020
Fixes SWDEV-223910 and SWDEV-223663
mangupta pushed a commit that referenced this pull request Mar 13, 2020
Fixes SWDEV-223910 and SWDEV-223663

Change-Id: I0726cd5aacd656bc56c4ba38e4af3da83f5a92db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs_updates PR initially approved. But needs rework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants