[Openmp-commits] [openmp] [Flang][OpenMP][MLIR] Remove deletion of unused declare target global after use replacement (PR #67762)

[via Openmp-commits]

https://github.com/agozillon created https://github.com/llvm/llvm-project/pull/67762

At the moment, for device a reference pointer is generated in place of the original declare target global value, this reference pointer is the pointer that actually receives the data. In Clang the original global value isn't generated for device, just the reference pointer.

Unfortunately for Flang/MLIR this is currently not the case, as the declare target attribute is processed after the creation of the global so we end up with a dead global on device effectively after rewriting its uses to the new device reference pointer.

It appears I was a little overzealous with the deletion of the declare target globals for device. The current method breaks in-cases where the same declare target global is used across two target regions (added a runtime reproduced in the patch). As it'll effectively delete it before the second target gets a chance to be written to LLVM IR and have it's uses rewritten .

I'd like to remove this deletion as the dead global isn't breaking any code and will likely be removed in later dead code elimination passes, perhaps a little too heavy handed with the original approach.

>From 0508b13895d30a7464fcca87cae9effb9d144e7a Mon Sep 17 00:00:00 2001
From: Andrew Gozillon <Andrew.Gozillon at amd.com>
Date: Thu, 28 Sep 2023 22:36:27 -0500
Subject: [PATCH] [Flang][OpenMP][MLIR] Remove deletion of unused declare
 target global after use replacement

At the moment, for device a reference pointer is generated in place of
the original declare target global value, this reference pointer is the
pointer that actually recieves the data. In Clang the original global
value isn't generated for device, just the reference pointer.

Unfortuntely for Flang/MLIR this is currently not the case, as the
declare target attribtue is processed after the creation of the
global so we end up with a dead global on device effectively
after rewriting its uses to the new device reference pointer.

It appears I was a little overzealous with the deletion of the declare
target globals for device. The current method breaks in-cases where
the same declare target global is used across two target regions
(added a runtime reproduced in the patch). As it'll effectively
delete it before the second target gets a chance to be written to
LLVM IR and have it's uses rewritten .

I'd like to remove this deletion as the dead global isn't breaking
any code and will likely be removed in later dead code
elimination passes.
---
 .../OpenMP/OpenMPToLLVMIRTranslation.cpp      |  9 ----
 ...double-target-call-with-declare-target.f90 | 43 +++++++++++++++++++
 2 files changed, 43 insertions(+), 9 deletions(-)
 create mode 100644 openmp/libomptarget/test/offloading/fortran/double-target-call-with-declare-target.f90

diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 8f7f1963b3e5a4f..14bcbc3018f72bd 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1989,15 +1989,6 @@ handleDeclareTargetMapVar(llvm::ArrayRef<Value> mapOperands,
           user->replaceUsesOfWith(mapOpValue, load);
         }
       }
-
-      // A global has already been generated by this stage for device
-      // that's now dead after the remapping, we can remove it now,
-      // as we have replaced all usages with the new ref pointer.
-      if (llvm::GlobalValue *unusedGlobalVal =
-              dyn_cast<llvm::GlobalValue>(mapOpValue)) {
-        unusedGlobalVal->dropAllReferences();
-        unusedGlobalVal->eraseFromParent();
-      }
     }
   }
 }
diff --git a/openmp/libomptarget/test/offloading/fortran/double-target-call-with-declare-target.f90 b/openmp/libomptarget/test/offloading/fortran/double-target-call-with-declare-target.f90
new file mode 100644
index 000000000000000..b4c793ca06cf798
--- /dev/null
+++ b/openmp/libomptarget/test/offloading/fortran/double-target-call-with-declare-target.f90
@@ -0,0 +1,43 @@
+! Offloading test with two target regions mapping the same
+! declare target Fortran array and writing some values to 
+! it before checking the host correctly receives the 
+! correct updates made on the device.
+! REQUIRES: flang, amdgcn-amd-amdhsa
+! UNSUPPORTED: nvptx64-nvidia-cuda
+! UNSUPPORTED: nvptx64-nvidia-cuda-LTO
+! UNSUPPORTED: aarch64-unknown-linux-gnu
+! UNSUPPORTED: aarch64-unknown-linux-gnu-LTO
+! UNSUPPORTED: x86_64-pc-linux-gnu
+! UNSUPPORTED: x86_64-pc-linux-gnu-LTO
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+module test_0
+    implicit none
+    integer :: sp(10) = (/0,0,0,0,0,0,0,0,0,0/)
+    !$omp declare target link(sp)
+end module test_0
+
+program main
+    use test_0
+    integer :: i = 1
+    integer :: j = 11
+
+!$omp target map(tofrom:sp) map(to: i, j)
+    do while (i <= j)
+        sp(i) = i;
+        i = i + 1
+    end do
+!$omp end target
+
+!$omp target map(tofrom:sp) map(to: i, j)
+    do while (i <= j)
+        sp(i) = sp(i) + i;
+        i = i + 1
+    end do
+!$omp end target
+    
+print *, sp(:)
+
+end program
+
+! CHECK: 2 4 6 8 10 12 14 16 18 20