-
Notifications
You must be signed in to change notification settings - Fork 15k
[OpenMP] Adds omp_target_is_accessible routine #138294
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
Changes from 35 commits
025d36e
b33b27e
bf01578
d20f4d5
cb87242
03d45cc
21b1d6a
95ab6fe
2436211
c92c94f
eeb7604
9d97424
34acf27
d4ecaf6
2792290
712bdd1
dd15747
108e4b9
e9dccd6
4345232
4b51745
0ef2e79
454df9e
79dd36f
0ccae1d
588c394
b751e75
584553e
8961e19
357ccd9
f271422
4a223a4
464235c
d84d07b
ed8cf2d
af149ce
e8838d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,6 +196,34 @@ EXTERN int omp_target_is_present(const void *Ptr, int DeviceNum) { | |
| return Rc; | ||
| } | ||
|
|
||
| /// Check whether a pointer is accessible from a device. | ||
| /// Returns true when accessibility is guaranteed otherwise returns false. | ||
| EXTERN int omp_target_is_accessible(const void *Ptr, size_t Size, | ||
| int DeviceNum) { | ||
| TIMESCOPE(); | ||
| OMPT_IF_BUILT(ReturnAddressSetterRAII RA(__builtin_return_address(0))); | ||
| DP("Call to omp_target_is_accessible for device %d, address " DPxMOD | ||
| ", size %zu\n", | ||
| DeviceNum, DPxPTR(Ptr), Size); | ||
|
|
||
| if (!Ptr) { | ||
| DP("Call to omp_target_is_accessible with NULL ptr returning false\n"); | ||
nicebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return false; | ||
| } | ||
|
|
||
| if (DeviceNum == omp_get_initial_device() || DeviceNum == -1) { | ||
| DP("Call to omp_target_is_accessible on host, returning true\n"); | ||
| return true; | ||
| } | ||
|
|
||
| // The device number must refer to a valid device | ||
| auto DeviceOrErr = PM->getDevice(DeviceNum); | ||
| if (!DeviceOrErr) | ||
| FATAL_MESSAGE(DeviceNum, "%s", toString(DeviceOrErr.takeError()).c_str()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder whether this is a fatal message or simple just return false? What does the spec say when the device number is invalid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @shiltian : let's return false if the device doesn't exist. A warning is also in order. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this change but then we probably need to reconsider how we handle invalid devices in all cases in the runtime since in the API.cpp file alone it's handled in this way for: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a question to spec. If spec doesn't say anything, my $.02 is to return false. We probably don't want a program to crash immediately with invalid input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That doesn't change the fact that we have those other functions that don't state that the program is to crash immediately with invalid input. Do we then need to adjust those functions as well to return false and emit a warning? I don't think it's sensible to have one function act in one way and one in another if the device is invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes indeed. We definitely don't want inconsistent behavior. We need to update them all (in a separate PR for others which may also server as an RFC). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think crashing is the better behavior for several reasons.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that is different. For a target region, yes, crash is okay, since the spec doesn't seem to say anything about how to deal with invalid input in clauses, based on my understanding (might be out of date though). APIs, on the other hand, is different. I think it'd not be a good idea someone calls some functions with invalid input and then their program crashes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a discussion we need to have in another setting than this issue as it does not just affect this API routine but existing routines (of which I stated a couple of examples above). My implementation follows the way this is currently handled so I think if you want to change the handling the discussion needs to be had in a different setting with a bigger audience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. CC @jhuber6 Maybe a topic for the next Wed meeting. :-) |
||
|
|
||
| return DeviceOrErr->isAccessiblePtr(Ptr, Size); | ||
| } | ||
|
|
||
| EXTERN int omp_target_memcpy(void *Dst, const void *Src, size_t Length, | ||
| size_t DstOffset, size_t SrcOffset, int DstDevice, | ||
| int SrcDevice) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3062,6 +3062,30 @@ struct AMDGPUDeviceTy : public GenericDeviceTy, AMDGenericDeviceTy { | |
| return ((IsAPU || OMPX_ApuMaps) && IsXnackEnabled); | ||
| } | ||
|
|
||
| Expected<bool> isAccessiblePtrImpl(const void *Ptr, size_t Size) override { | ||
| hsa_amd_pointer_info_t Info; | ||
| Info.size = sizeof(hsa_amd_pointer_info_t); | ||
|
|
||
| hsa_agent_t *Agents = nullptr; | ||
| uint32_t Count = 0; | ||
| hsa_status_t Status = | ||
| hsa_amd_pointer_info(Ptr, &Info, malloc, &Count, &Agents); | ||
|
|
||
| if (auto Err = Plugin::check(Status, "error in hsa_amd_pointer_info: %s")) | ||
| return std::move(Err); | ||
|
|
||
| // Checks if the pointer is known by HSA and accessible by the device | ||
| for (uint32_t i = 0; i < Count; i++) { | ||
| if (Agents[i].handle == getAgent().handle) | ||
| return Info.sizeInBytes >= Size; | ||
| } | ||
|
|
||
| // If the pointer is unknown to HSA it's assumed a host pointer | ||
| // in that case the device can access it on unified memory support is | ||
| // enabled | ||
| return IsXnackEnabled; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a nit: there are GPUs that do not have xnack, but still have the ability to access host memory. This behavior is fine for now, but we will have to revisit based on GPU (later, not in this PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it should be documented. |
||
| } | ||
|
|
||
| /// Getters and setters for stack and heap sizes. | ||
| Error getDeviceStackSize(uint64_t &Value) override { | ||
| Value = StackSize; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // RUN: %libomptarget-compilexx-generic | ||
| // RUN: env HSA_XNACK=1 %libomptarget-run-generic 2>&1 \ | ||
| // RUN: | %fcheck-generic | ||
|
|
||
| // RUN: %libomptarget-compilexx-generic | ||
| // RUN: env HSA_XNACK=0 %libomptarget-run-generic 2>&1 \ | ||
| // RUN: | %fcheck-generic -check-prefix=NO_USM | ||
|
|
||
| // REQUIRES: unified_shared_memory | ||
| // XFAIL: nvptx | ||
|
|
||
| // CHECK: SUCCESS | ||
| // NO_USM: Not accessible | ||
|
|
||
| #include <assert.h> | ||
| #include <iostream> | ||
| #include <omp.h> | ||
| #include <stdio.h> | ||
|
|
||
| int main() { | ||
| int n = 10000; | ||
| int *a = new int[n]; | ||
| int err = 0; | ||
|
|
||
| // program must be executed with HSA_XNACK=1 | ||
| if (!omp_target_is_accessible(a, n * sizeof(int), /*device_num=*/0)) | ||
| printf("Not accessible\n"); | ||
| else { | ||
| #pragma omp target teams distribute parallel for | ||
| for (int i = 0; i < n; i++) | ||
| a[i] = i; | ||
|
|
||
| for (int i = 0; i < n; i++) | ||
| if (a[i] != i) | ||
| err++; | ||
| } | ||
|
|
||
| printf("%s\n", err == 0 ? "SUCCESS" : "FAIL"); | ||
| return err; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.