LLNL / RAJA

RAJA Performance Portability Layer (C++)

BSD 3-Clause "New" or "Revised" License

450 stars 102 forks source link

Add OpenMP 5.1 implementations for atomics #1666

Closed adayton1 closed 3 weeks ago

adayton1 commented 1 month ago

Summary

This PR is a feature and bugfix
It does the following:
- Adds OpenMP 5.1 implementations for atomicMin and atomicMax
- Fixes logic error in sequential atomicDec
- Prefer operator< instead of operator>
- Cast literals to appropriate type

adayton1 commented 4 weeks ago

Looks like I need to work out some issues still.

adayton1 commented 4 weeks ago

Can we drop support for intel 19 on ruby? It is hitting an internal compiler error.

adayton1 commented 4 weeks ago

I think there's still an issue with the openmp builds on tioga, but I'm removing the draft status so I can check on the CI results.

adayton1 commented 3 weeks ago

Can we drop support for intel 19 on ruby? It is hitting an internal compiler error.

I fixed the build on tioga by removing a few of the implementations I put in that I believe were incorrect after further review of the OpenMP standard, and the build errors on intel 19 disappeared.

rhornung67 commented 3 weeks ago

@adayton1 we will be updating our CI jobs soon. we will be dropping Intel 19, updating to intel/2023.2.1, and updating ROCm to 6.1.2. No ETA yet.

adayton1 commented 3 weeks ago

This is ready for review.

rhornung67 commented 3 weeks ago

@adayton1 note that we do not currently have openmp target ci testing in RAJA. This is something we need to add. Have you tested this on an LC system? If so, which platform and compiler?

adayton1 commented 3 weeks ago

@adayton1 note that we do not currently have openmp target ci testing in RAJA. This is something we need to add. Have you tested this on an LC system? If so, which platform and compiler?

On LC, it seems the only CZ CI platform with OpenMP 5.1 support is tioga. These implementations work there using OpenMP for CPU threading (assuming the CI testing launches more than one thread for OpenMP tests). I have not yet tested openmp target offload.

rhornung67 commented 3 weeks ago

@adayton1 note that we do not currently have openmp target ci testing in RAJA. This is something we need to add. Have you tested this on an LC system? If so, which platform and compiler?

On LC, it seems the only CZ CI platform with OpenMP 5.1 support is tioga. These implementations work there using OpenMP for CPU threading (assuming the CI testing launches more than one thread for OpenMP tests). I have not yet tested openmp target offload.

@adayton1 that's fair. As long as everything is guarded properly based on the OpenMP standard version, we should be good. It looks like you've done this.

trws commented 3 weeks ago

This looks good, it should also work on the newer intel compilers IIRC, but I'm not sure we have those tied into CI for anything but sycl right now.