[OpenMP][Offloading][AMDGPU] Exclude nogpulib in lit.cfg
#76702
Conversation
llvmbot
added
mlir
mlir:openmp
flang:openmp
openmp:libomptarget
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir Author: Kareem Ergawy (ergawy) ChangesIn order to run offloading tests for AMDGPUs, we should not use the This is a follow-up to #76355, please review only the last commit. Full diff: https://github.com/llvm/llvm-project/pull/76702.diff 3 Files Affected:
diff --git a/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp b/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
index cd1cfb3b7686d0..730858ffc67a71 100644
--- a/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
+++ b/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
@@ -239,11 +239,11 @@ void mlir::configureOpenMPToLLVMConversionLegality(
target.addDynamicallyLegalOp<
mlir::omp::AtomicReadOp, mlir::omp::AtomicWriteOp, mlir::omp::FlushOp,
mlir::omp::ThreadprivateOp, mlir::omp::YieldOp, mlir::omp::EnterDataOp,
- mlir::omp::ExitDataOp, mlir::omp::DataBoundsOp, mlir::omp::MapInfoOp>(
- [&](Operation *op) {
- return typeConverter.isLegal(op->getOperandTypes()) &&
- typeConverter.isLegal(op->getResultTypes());
- });
+ mlir::omp::ExitDataOp, mlir::omp::UpdateDataOp, mlir::omp::DataBoundsOp,
+ mlir::omp::MapInfoOp>([&](Operation *op) {
+ return typeConverter.isLegal(op->getOperandTypes()) &&
+ typeConverter.isLegal(op->getResultTypes());
+ });
target.addDynamicallyLegalOp<mlir::omp::ReductionOp>([&](Operation *op) {
return typeConverter.isLegal(op->getOperandTypes());
});
@@ -282,6 +282,7 @@ void mlir::populateOpenMPToLLVMConversionPatterns(LLVMTypeConverter &converter,
RegionLessOpConversion<omp::YieldOp>,
RegionLessOpConversion<omp::EnterDataOp>,
RegionLessOpConversion<omp::ExitDataOp>,
+ RegionLessOpConversion<omp::UpdateDataOp>,
RegionLessOpWithVarOperandsConversion<omp::DataBoundsOp>>(converter);
}
diff --git a/openmp/libomptarget/test/lit.cfg b/openmp/libomptarget/test/lit.cfg
index 19c5e5c4572227..dca922dcfc6bce 100644
--- a/openmp/libomptarget/test/lit.cfg
+++ b/openmp/libomptarget/test/lit.cfg
@@ -132,7 +132,7 @@ elif config.operating_system == 'Darwin':
config.test_flags += " -Wl,-rpath," + config.library_dir
config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
else: # Unices
- if config.libomptarget_current_target != "nvptx64-nvidia-cuda":
+ if config.libomptarget_current_target != "nvptx64-nvidia-cuda" and config.libomptarget_current_target != "amdgcn-amd-amdhsa":
config.test_flags += " -nogpulib"
config.test_flags += " -Wl,-rpath," + config.library_dir
config.test_flags += " -Wl,-rpath," + config.omp_host_rtl_directory
diff --git a/openmp/libomptarget/test/offloading/fortran/target_update.f90 b/openmp/libomptarget/test/offloading/fortran/target_update.f90
new file mode 100644
index 00000000000000..fb35c5a36ab0e5
--- /dev/null
+++ b/openmp/libomptarget/test/offloading/fortran/target_update.f90
@@ -0,0 +1,50 @@
+! Offloading test for the `target update` directive.
+
+! REQUIRES: flang, amdgcn-amd-amdhsa
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+program target_update
+ implicit none
+ integer :: x(1)
+ integer :: host_id
+ integer :: device_id(1)
+
+ INTERFACE
+ FUNCTION omp_get_device_num() BIND(C)
+ USE, INTRINSIC :: iso_c_binding, ONLY: C_INT
+ integer :: omp_get_device_num
+ END FUNCTION omp_get_device_num
+ END INTERFACE
+
+ x(1) = 5
+ host_id = omp_get_device_num()
+
+!$omp target enter data map(to:x, device_id)
+!$omp target
+ x(1) = 42
+!$omp end target
+
+ ! Test that without a `target update` directive, the target update to x is
+ ! not yet seen by the host.
+ ! CHECK: After first target regions and before target update: x = 5
+ print *, "After first target regions and before target update: x =", x(1)
+
+!$omp target
+ x(1) = 84
+ device_id(1) = omp_get_device_num()
+!$omp end target
+!$omp target update from(x, device_id)
+
+ ! Test that after the `target update`, the host can see the new x value.
+ ! CHECK: After second target regions and target update: x = 84
+ print *, "After second target regions and target update: x =", x(1)
+
+ ! Make sure that offloading to the device actually happened. This way we
+ ! verify that we didn't take the fallback host execution path.
+ ! CHECK: Offloading succeeded!
+ if (host_id /= device_id(1)) then
+ print *, "Offloading succeeded!"
+ else
+ print *, "Offloading failed!"
+ end if
+end program target_update
|
In order to run offloading tests for AMDGPUs, we should not use the `nogpulib` flag added by `lit.cfg`. This is already done for Nvidia GPUs and seems to have been overlooked for AMD.
ergawy
force-pushed
the
omp_target_upate_5
branch
from
da36502 to
a64fb16
Compare
ergawy
changed the title
nogpulib in lit.cfgnogpulib in lit.cfg
|
This was not an oversight, Nvidia (non-LTO version) was excluded because it doesn't use the static library version of the runtime. What failures is this causing? The initial concern was that without Another thing I've considered is instead of doing |
These are the failing tests: And they fail with the following error:
Apologies for assuming that without much background. From the name of the flag, I (possibly incorrectly) assumed that if we pass it to flang, then the device RT lib(s) won't be linked into the final binary which, in turn, will prevent offloading from taking place. That's why, I thought that excluding this flag for the The flag is not supported by flang (as evident from the error message), and there is a PR (#71045) to add it. However, I think adding it might not be the solution for these particular offloading tests. Please let me know if I missed something. |
|
Adding that flag makes sense in Fortran since I'm assuming there will be similar cases where users may wish to disallow the implicit libraries and include paths. However, if you want a quick fix I'd just suggest making some flag in the |
|
I will abandon this since @DominikAdamski has a proper solution for the problem in #71045 & #76796. |
In order to run offloading tests for AMDGPUs, we should not use the
nogpulibflag added bylit.cfg. This is already done for Nvidia GPUsand seems to have been overlooked for AMD.
This is a follow-up to #76355, please review only the last commit.