Glad to see more error traps.

Please also include the environment variable in the patch summary.

In D145329#4411300, @arsenm wrote:

In D145329#4410519, @ye-luo wrote:

ping @arsenm could you finalize this patch and close https://github.com/llvm/llvm-project/issues/61083?

Testing found a new crash in blender so I need to look at this more

ping @arsenm could you finalize this patch and close https://github.com/llvm/llvm-project/issues/61083?

I don't remember what workload was choked on this. My quick testing shows everything being fine. So let us just land this.

Updating the patch description is also needed.

From the meeting discussion.

1. default to active waiting rather than 0.
2. Consider a single LIBOMPTARGET_AMDGPU_STREAM_BUSYWAIT.

In D148808#4286873, @gregrodgers wrote:

In D148808#4283842, @ye-luo wrote:

Why separate KERNEL_BUSYWAIT and DATA_BUSYWAIT? I don't feel the need of two separate controls.
What are the units? Documentation?

It is a matter of priorities. If you are doing async copies, you may not want them to every busy-wait because you have more important things to do on the CPU.

Why separate KERNEL_BUSYWAIT and DATA_BUSYWAIT? I don't feel the need of two separate controls.
What are the units? Documentation?

Currently, we only load an image if its supported architecture matches all of the devices found.

Cannot understand

Unfortunately my original problem still persists with this patch. The active wait timed out and then still stuck in the blocking wait.
Tested 3, 30, 300, 3000, 30000, 300000, 3000000 on my W6800. I have to consider some software between hsa and the hardware is broken.

Got compiler warning.

/scratch3/opt/llvm-clang/llvm-project/openmp/libomptarget/src/omptarget.cpp: In lambda function:
/scratch3/opt/llvm-clang/llvm-project/openmp/libomptarget/src/omptarget.cpp:1058:56: warning: cast from type 'void* const*' to type 'void*' casts away qualifiers [-Wcast-qual]
 1058 |                                     (void *)&ShadowPtr.TgtPtrVal,
      |

Got test failure

Failed Tests (1):
  libomptarget :: x86_64-pc-linux-gnu :: offloading/barrier_fence.c

Keep things moving.

@JonChesterfield is this patch good to go?

Still prefer a more clear message.

My understanding from the weekly call is that "locked" is for a device agent not the device.
It can be used in hsa memory copy calls but is_device_ptr is for device(kernel) access and thus incorrect.

In D142514#4078989, @tianshilei1992 wrote:

If it servers as notifying data mapping, __tgt_rtl_notify_data_map or something like that sounds more appropriate to me. __tgt_rtl_data_mapped sounds like checking whether it is mapped, and that's how it confused me initially.

It doesn't print well

$ OMP_TARGET_OFFLOAD=mandatory ./a.out 
Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options.
Libomptarget error: None of the images ( gfx90a 
Libomptarget error: Source location information not present. Compile with -g or -gline-tables-only.
Libomptarget fatal error 1: failure of target construct while offloading is mandatory

In D142399#4074871, @kevinsala wrote:

In D142399#4074765, @jhuber6 wrote:

@carlo.bertolli @kevinsala The only tests the nextgen plugins fail for me are the following. Any update on getting this resolved? I think we should aim to land this before the fork and then work on deleting the old plugins after.

Failed Tests (2):
  libomptarget :: amdgcn-amd-amdhsa :: mapping/prelock.cpp
  libomptarget :: amdgcn-amd-amdhsa-LTO :: mapping/prelock.cpp

The patch D141227 implements the lock/unlock API. Yesterday I updated the patch with new features. We can merge it if you all find it OK.

Regarding the tests, even with the previous patch applied, they fail in the code line below. Is that line actually valid? We are trying to map an address (locked) that cannot be accessed by the host. That pointer is the agent_ptr returned by hsa_amd_memory_lock. If the plugin tries to access that buffer it will segfault. That's what is probably happening in the nextgen plugin because it tries to call std::memcpy on that "host pointer" in __tgt_rtl_data_submit.

https://github.com/llvm/llvm-project/blob/main/openmp/libomptarget/test/mapping/prelock.cpp#L39

In D142369#4074095, @jhuber6 wrote:

In D142369#4074092, @ye-luo wrote:

When libomptarget failed to load the amd plugin due to missing hsa runtime so files.
I got

yeluo@epyc-server:~$ clang++ -fopenmp --offload-arch=gfx90a main.cpp
yeluo@epyc-server:~$ OMP_TARGET_OFFLOAD=mandatory ./a.out 
Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options.
Libomptarget error: None of the images found are compatible: gfx90a
Libomptarget error: Source location information not present. Compile with -g or -gline-tables-only.
Libomptarget fatal error 1: failure of target construct while offloading is mandatory

None of the images found are compatible: gfx90a this line is a bit hard to digest. Could you print a list of loaded plugins and a list of images.

The following is the list of images found. I could try to make that more explicit. And by default we should have every plugin loaded.

Ref-counting lock/unlock makes a lot sense.

When libomptarget failed to load the amd plugin due to missing hsa runtime so files.
I got

yeluo@epyc-server:~$ clang++ -fopenmp --offload-arch=gfx90a main.cpp
yeluo@epyc-server:~$ OMP_TARGET_OFFLOAD=mandatory ./a.out 
Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options.
Libomptarget error: None of the images found are compatible: gfx90a
Libomptarget error: Source location information not present. Compile with -g or -gline-tables-only.
Libomptarget fatal error 1: failure of target construct while offloading is mandatory

How does the above message behaves when there is no GPU on the node.

This patch needs a rebase. Applying patch failed.

In D141227#4071198, @JonChesterfield wrote:

I think it's good to go. If this is something you've tried in qmcpack so we have a real world example of it improving things then even better.

Ping. What is left before merging?

Quite close to the release. Let's merge this and deal with the remaining issue in a later patch.

In D139208#4041427, @carlo.bertolli wrote:

In D139208#4041132, @ye-luo wrote:

lock_memory and unlock_memory not symmetric.

I mean
lock_memory(ptr) # lock happens
lock_memory(ptr) # no-op
unlock_memory(ptr) # unlock happens
unlock_memory(ptr) # no-op

I just feel unsafe. Feel more safe if
lock_memory(ptr) # lock happens
lock_memory(ptr) # no-op
unlock_memory(ptr) # no-op
unlock_memory(ptr) # unlock happens
but this requires doing reference counting.

I've just tested this in a pure C++ (no openmp) program using ROCr directly.
Here's the result of the operations above (I am not sure we are doing no-op):
a is pinned after first lock
a is pinned after second lock
a is pinned after first unlock
a is not pinned after second unlock

Code (please fill the void as necessary, or I can provide actual file if you want to test locally):

double *a = (double *)aligned_alloc(4096, n*sizeof(double)); //new double[n];
double *b = (double *)aligned_alloc(4096, n*sizeof(double)); //new double[n];

double *pinned_a = nullptr;
err = hsa_amd_memory_lock((void *) a, n*sizeof(double), nullptr, 0, (void **) &pinned_a);
CHECK(err);

{
  bool already_pinned = false;
  hsa_amd_pointer_info_t info;
  info.size = sizeof(hsa_amd_pointer_info_t);
  err = hsa_amd_pointer_info(a, &info, nullptr, nullptr, nullptr);
  CHECK(err);
  already_pinned = (info.type == HSA_EXT_POINTER_TYPE_LOCKED);
  if (already_pinned) printf("a is pinned after first lock\n");
  else printf("a is not pinned after first lock\n");
}

double *repinned_a = nullptr;
err = hsa_amd_memory_lock((void *) a, n*sizeof(double), nullptr, 0, (void **) &repinned_a);
CHECK(err);

{
  bool already_pinned = false;
  hsa_amd_pointer_info_t info;
  info.size = sizeof(hsa_amd_pointer_info_t);
  err = hsa_amd_pointer_info(a, &info, nullptr, nullptr, nullptr);
  CHECK(err);
  already_pinned = (info.type == HSA_EXT_POINTER_TYPE_LOCKED);
if (already_pinned) printf("a is pinned after second lock\n");
else printf("a is pinned after second lock\n");
}


hsa_amd_memory_unlock(pinned_a);
{
  bool already_pinned = false;
  hsa_amd_pointer_info_t info;
  info.size = sizeof(hsa_amd_pointer_info_t);
  err = hsa_amd_pointer_info(a, &info, nullptr, nullptr, nullptr);
  already_pinned = (info.type == HSA_EXT_POINTER_TYPE_LOCKED);
  if (already_pinned) printf("a is pinned after first unlock\n");
  else printf("a is not pinned after first unlock\n");
}

{
  bool already_pinned = false;
  hsa_amd_pointer_info_t info;
  info.size = sizeof(hsa_amd_pointer_info_t);
  hsa_amd_memory_unlock(a);
  err = hsa_amd_pointer_info(a, &info, nullptr, nullptr, nullptr);
  already_pinned = (info.type == HSA_EXT_POINTER_TYPE_LOCKED);
  if (already_pinned) printf("a is pinned after second unlock\n");
  else printf("a is not pinned after second unlock\n");
}

This looks like what you want, right?

lock_memory and unlock_memory not symmetric.

I mean
lock_memory(ptr) # lock happens
lock_memory(ptr) # no-op
unlock_memory(ptr) # unlock happens
unlock_memory(ptr) # no-op

I still don't feel safe about the amd plugin implementation

1. how is_locked handle error.
2. lock_memory doesn't return error code
3. lock_memory and unlock_memory behavior is not symmetric.

But that is only inside the plugin.

Does this patch work when there are more than one level of inheritance? For example class B: public A; class C: public B

ping. I'm looking forward to the landing of this patch.

All the lock functions should return pointers via an argument.
All the lock and unlock functions should return error codes.

Got tons of runtime failure

target doesn't support jit
UNREACHABLE executed at /gpfs/jlse-fs0/users/yeluo/opt/llvm-clang/llvm-project-nightly/openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:543!

on AMD GPU gfx906 when running miniqmc ctest
failure happens with
LIBOMPTARGET_NEXTGEN_PLUGINS=1

My requests have been addressed.

clang-formatting

Seems fine in my builds of OpenMP as a project and a runtime.

Pass tests on my side. Thanks