Skip to content

Conversation

jplehr
Copy link
Contributor

@jplehr jplehr commented Sep 30, 2025

The test covers some of the identifier symbols in the interop runtime.

This test, for now, is to guard against complete breakage, which was the result of the other interop.c test not being enabled on AMD and thus, not caught by our buildbots.
It can certainly be improved. :)

Depends on #161429 to restore functionality.

@jplehr jplehr requested review from jhuber6 and mjklemm September 30, 2025 20:30
@llvmbot
Copy link
Member

llvmbot commented Sep 30, 2025

@llvm/pr-subscribers-offload

Author: Jan Patrick Lehr (jplehr)

Changes

The test covers some of the identifier symbols in the interop runtime.

This test, for now, is to guard against complete breakage, which was the result of the other interop.c test not being enabled on AMD and thus, not caught by our buildbots.
It can certainly be improved. :)

Depends on #161429 to restore functionality.


Full diff: https://github.com/llvm/llvm-project/pull/161434.diff

1 Files Affected:

  • (added) offload/test/offloading/interop-print.c (+74)
diff --git a/offload/test/offloading/interop-print.c b/offload/test/offloading/interop-print.c
new file mode 100644
index 0000000000000..a474501649777
--- /dev/null
+++ b/offload/test/offloading/interop-print.c
@@ -0,0 +1,74 @@
+// RUN: %libomptarget-compile-amdgcn-amd-amdhsa
+// RUN:   %libomptarget-run-generic 2>&1 | \
+// RUN:   %fcheck-generic -check-prefix=AMDCHECK
+
+// REQUIRES: amdgpu
+
+#include <omp.h>
+#include <stdio.h>
+
+const char* interop_int_to_string( const int interop_int )
+{
+    switch( interop_int )
+    {
+        case 1:
+            return "cuda";
+        case 2:
+            return "cuda_driver";
+        case 3:
+            return "opencl";
+        case 4:
+            return "sycl";
+        case 5:
+            return "hip";
+        case 6:
+            return "level_zero";
+        case 7:
+            return "hsa";
+        default:
+            return "unknown";
+    }
+}
+
+int main( int argc, char** argv )
+{
+    omp_interop_t iobj = omp_interop_none;
+    #pragma omp interop init(targetsync: iobj)
+
+    int err;
+    int interop_int = omp_get_interop_int( iobj, omp_ipr_fr_id, &err );
+
+    if( err )
+    {
+        fprintf( stderr, "omp_get_interop_int failed: %d\n", err );
+        return -1;
+    }
+
+  // FIXME: This should be hsa instead of hip
+  // AMDCHECK: {{.*}} hip
+    printf( "omp_get_interop_int returned %s\n", interop_int_to_string( interop_int ) );
+
+    const char* interop_vendor = omp_get_interop_str( iobj, omp_ipr_vendor_name, &err );
+    if( err )
+    {
+        fprintf( stderr, "omp_get_interop_str failed: %d\n", err );
+        return -1;
+    }
+
+  // AMDCHECK: {{.*}} amd
+    printf( "omp_get_interop_str returned %s\n", interop_vendor );
+
+    const char* interop_fr_name = omp_get_interop_str( iobj, omp_ipr_fr_name, &err );
+    if( err )
+    {
+        fprintf( stderr, "omp_get_interop_str failed: %d\n", err );
+        return -1;
+    }
+
+  // FIXME: This should be hsa instead of hip
+  // AMDCHECK: {{.*}} hip
+    printf( "omp_get_interop_str returned %s\n", interop_fr_name );
+
+    #pragma omp interop destroy(iobj)
+    return 0;
+}

Copy link

github-actions bot commented Sep 30, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

The test covers some of the identifier symbols in the interop runtime.
@jplehr jplehr force-pushed the nfc/add-interop-print-test branch from 45b7bab to 54544d9 Compare September 30, 2025 20:42
The required linked PR got updated to include that fix. Adjusting the
test to reflect the correct behavior.
@Thyre
Copy link
Contributor

Thyre commented Oct 2, 2025

Thanks for creating the PR to add a test for this 😄

Just for my understanding:

We add a test to ensure that the strings for AMDGPU are correct. Looking at the already existing test for NVIDIA/CUDA in offload/test/offloading/interop.c, we never check the returned values and basically just try to ensure that the calls don't crash (with one additional test to check if we got a CUDA stream from the OpenMP runtime), right? Should we add similar tests for Intel & NVIDIA as well?

@adurang
Copy link
Contributor

adurang commented Oct 2, 2025

Just wondering if there's a way to have this test enabled for all plugins and not just AMD (as otherwise we would need the same for each other plugin at least?). Like having another RUN command for NVIDIA with a different prefix for example?

Also, I'd suggest to loop through all devices and not just the default device:

for ( int id = 0; id< omp_get_num_devices(); id++ ) {
  omp_interop_t iop;
  #pragma omp interop init(targesync:iop) device(id)
  ...
}


int main(int argc, char **argv) {
omp_interop_t iobj = omp_interop_none;
#pragma omp interop init(targetsync : iobj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the usage in this test, I think it should of type target not targetsync:

Suggested change
#pragma omp interop init(targetsync : iobj)
#pragma omp interop init(target : iobj)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making this change gives me a seg fault during execution

@jplehr
Copy link
Contributor Author

jplehr commented Oct 2, 2025

We add a test to ensure that the strings for AMDGPU are correct.

And to run an interop test on AMDGPU while we bring up the functionality required to make interop.c work. Mostly to prevent unintended build breakages with what's currently there.

Should we add similar tests for Intel & NVIDIA as well?

I think we should. And we can include that in this test via nvidia-RUN lines. I don't think we have Intel in-tree as of now?

Just wondering if there's a way to have this test enabled for all plugins and not just AMD

Absolutely. I mostly cannot test it easily on other platforms.

I can add the RUN lines but someone would need to validate that the strings match, or add the checked strings for other platforms as a follow-up.

@Thyre
Copy link
Contributor

Thyre commented Oct 2, 2025

Absolutely. I mostly cannot test it easily on other platforms.

I can add the RUN lines but someone would need to validate that the strings match, or add the checked strings for other platforms as a follow-up.

I'll start a build of #161429 on a machine so that I can provide the strings for NVIDIA. Don't know if Intel would work before #158900 is merged.

jplehr added 2 commits October 2, 2025 06:28
- Added Nvidia run line
- Added loop around all devices
@Thyre
Copy link
Contributor

Thyre commented Oct 2, 2025

The strings for NVIDIA look good:

$ clang --version
clang version 22.0.0git (https://github.com/adurang/llvm-project.git 6090c32a072bd97fc24709c91ffe6dde770e0577)
Target: aarch64-unknown-linux-gnu
Thread model: posix
InstalledDir: /p/project1/ccstpa/reuter1/Projects/Sources/LLVM/interop/llvm-project/_build/_install/bin
$ ./bin/clang test.c -fopenmp --offload-arch=sm_90 -frtlib-add-rpath -L$(pwd)/_install/lib/aarch64-unknown-linux-gnu -Wl,-rpath=$(pwd)/_install/lib/aarch64-unknown-linux-gnu
$ ./a.out
omp_get_interop_int returned cuda
omp_get_interop_str returned nvidia
omp_get_interop_str returned cuda
"PluginInterface" error: Failure to synchronize interop object 0x0000aaaaeccff8c0: "generic error code for features unsupported by the device/backend" sync_barrier not supported
omptarget fatal error 0: Interop sync barrier failed for 0xaaaaeccff8c0 object

Aborted (core dumped)

The error at the end should be expected if I understood the discussion in the other PRs correctly. Switching to target instead of targetsync still yields the same strings, but removes the error at the end.


Regarding multi-GPU with different archs, maybe I can check this but I can't guarantee that I get this to work.

Edit: Unfortunately not, as the AMD GPU on that system doesn't seem to work correctly at the moment.

@jplehr
Copy link
Contributor Author

jplehr commented Oct 2, 2025

Switching to target instead of targetsync still yields the same strings, but removes the error at the end.

Unfortunately that crashes on AMD. :)

@Thyre
Copy link
Contributor

Thyre commented Oct 2, 2025

Unfortunately that crashes on AMD. :)

In that case, we should just keep this as is. As you've said in #161429 (comment), we should implement the missing functionality regardless. So this would just be an additional test that fails for the same reason.

@jplehr
Copy link
Contributor Author

jplehr commented Oct 2, 2025

I think we should then either disable, or XFAIL it on Nvidia though. @jhuber6 just spent some time to make check-offload pass for him. Would not want to land this and bring in breaking tests knowingly.

@adurang
Copy link
Contributor

adurang commented Oct 2, 2025

Switching to target instead of targetsync still yields the same strings, but removes the error at the end.

Unfortunately that crashes on AMD. :)

That seems odd. I don't think targetsync should work for AMD in the current state of the plugin, and target should be the only one working rn. I feel like the type translation in the wrapper might be incorrect, let me check that.

adurang added a commit to adurang/llvm-project that referenced this pull request Oct 2, 2025
@jplehr jplehr requested a review from mhalk October 6, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants