[PATCH] D63108: [OpenMP] Add support for handling declare target to clause when unified memory is required

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/CodeGen/CGExpr.cpp:2297-2319
+static Address emitDeclTargetToVarDeclLValue(CodeGenFunction &CGF,
+                                             const VarDecl *VD, QualType T) {
+  llvm::Optional<OMPDeclareTargetDeclAttr::MapTypeTy> Res =
+      OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(VD);
+  if (!Res || *Res == OMPDeclareTargetDeclAttr::MT_Link)
+    return Address::invalid();
+  assert(*Res == OMPDeclareTargetDeclAttr::MT_To && "Expected to clause");
----------------
I think it would be better to merge these 2 functions into 1 `emitDeclTargetVarDeclLValue`. It should return the correct address for link vars and to vars with unified memory.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7458-7467
           if (*Res == OMPDeclareTargetDeclAttr::MT_Link) {
             IsLink = true;
             BP = CGF.CGM.getOpenMPRuntime().getAddrOfDeclareTargetLink(VD);
           }
+          if (*Res == OMPDeclareTargetDeclAttr::MT_To &&
+              CGF.CGM.getOpenMPRuntime().hasRequiresUnifiedSharedMemory()) {
+            // TODO: Make this into a flag for TO with unified memory.
----------------
You can merge those pieces of code into one, no need to duplicate. Plus, I don't think we need a new flag for to with unified memory if we can express everything using the existing flags.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1123-1127
+  virtual Address getAddrOfDeclareTargetToUnderUnifiedMem(const VarDecl *VD);
+
   /// Returns the address of the variable marked as declare target with link
   /// clause.
   virtual Address getAddrOfDeclareTargetLink(const VarDecl *VD);
----------------
Same here, better to merge these 2 functions into one `getAddrOfDeclareTargetVar`


================
Comment at: test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp:2
 // Test declare target link under unified memory requirement.
 // RUN: %clang_cc1 -verify -fopenmp -fopenmp-cuda-mode -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s --check-prefix CHECK
 // expected-no-diagnostics
----------------
`CHECK` is the default prefix, no need to specify it.


================
Comment at: test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp:2
 // Test declare target link under unified memory requirement.
 // RUN: %clang_cc1 -verify -fopenmp -fopenmp-cuda-mode -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s --check-prefix CHECK
 // expected-no-diagnostics
----------------
ABataev wrote:
> `CHECK` is the default prefix, no need to specify it.
We also need the checks for the device codegen since you're changing something not only in the host codegen, but in the device codegen too. Just extend this test to check for the codegen for the device.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63108/new/

https://reviews.llvm.org/D63108