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 unit test and modify map_to_peer api #21

Merged
merged 7 commits into from
Apr 13, 2016
Merged

Conversation

facao
Copy link
Contributor

@facao facao commented Apr 12, 2016

No description provided.

@@ -19,7 +19,7 @@ struct AmPointerInfo {
void * _hostPointer; ///< Host pointer. If host access is not allowed, NULL.
void * _devicePointer; ///< Device pointer.
size_t _sizeBytes; ///< Size of allocation.
hc::accelerator &_acc; ///< Device / Accelerator to use.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@facao and @bensander . after our discussion I think it's better to change this field to hc::accelerator* , instead of hc::accelerator& (so we don't have undefined semantic), or hc::accelerator (so we save memory).

Copy link
Collaborator

Choose a reason for hiding this comment

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

As @facao pointed out, hc::accelerator is just a wrapper of KalmarDevice* , and I've verified that sizeof(hc::accelerator) is only 8. So we should be fine copying it around.

@whchung
Copy link
Collaborator

whchung commented Apr 12, 2016

@facao Per our discussion I'm for the idea moving to std::shared_ptr<hc::accelerator> . This should be a coordinated change in both HIP and HCC. So I'd suggest you:

  • forfeit the current pull request.
  • submit a new one with only the test cases in, as they shouldn't be affected by the change in AmPointerInfo.
  • make the changes in both HIP and HCC, and submit new pull requests to both HIP and HCC, for the proposed change in AmPointerInfo.

@whchung
Copy link
Collaborator

whchung commented Apr 13, 2016

@facao There are conflicts in this pull request. Could you help resolve them so we can adopt this pull request?

@whchung whchung merged commit a4c8979 into ROCm:hsa_pool_api Apr 13, 2016
* accelerator is peer of itself.
* FIXME: on current system, dGPU is peer of any each
* other, we should expect is_get_peer() return true.
*/
Copy link
Collaborator

@whchung whchung Apr 15, 2016

Choose a reason for hiding this comment

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

@facao the unit test assume all dGPUs are peers to each other, which may be true for supported configuration. but for systems without P2P support acc.get_is_peer() would return false and this test would fail.

as the result of the test depends on the hardware configuration, and in many cases won't simply work, so I'd propose ways to improve this test case. And I'm more inclined to ask you improve the test as the 2nd method.

  1. relax this test so as long as long as acc.get_is_peer() could return a valid value, then call the test pass. this would reduce the test case to merely check whether the API is available in the header file.
  2. change the test case, and use HSA runtime API to query if the hardware do have HSA agents who are peers, and then check if the result of acc.get_is_peer() agree with it. You can refer to the following unit test as an example:

https://github.com/RadeonOpenCompute/hcc/blob/master/tests/Unit/AsyncPFE/accelerator_view_wait.cpp

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