[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/CGOpenMPRuntime.cpp:2566-2569
+      if (*Res == OMPDeclareTargetDeclAttr::MT_Link)
+        OS << CGM.getMangledName(GlobalDecl(VD)) << "_decl_tgt_link_ptr";
+      else
+        OS << CGM.getMangledName(GlobalDecl(VD)) << "_decl_tgt_to_ptr";
----------------
I think we can use the same suffix in all cases, instead of `_link_ptr` or `_to_ptr` just something like `_ref_ptr` would be good.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7465
     bool IsCaptureFirstInfo = IsFirstComponentList;
-    bool IsLink = false; // Is this variable a "declare target link"?
+    bool IsLinkOrToClause = false; // Is this variable a "declare target link"?
 
----------------
Fix the comment too. Also, the name of the variable is not very good, better to rename it to something like `MustBeReference` or something similar.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9230-9232
     Flags = OffloadEntriesInfoManagerTy::OMPTargetGlobalVarEntryLink;
+    if (*Res == OMPDeclareTargetDeclAttr::MT_To)
+      Flags = OffloadEntriesInfoManagerTy::OMPTargetGlobalVarEntryTo;
----------------
Better to have `if-else` here 


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9270
+      assert((*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+              *Res == OMPDeclareTargetDeclAttr::MT_To) &&
              "Expected to or link clauses.");
----------------
`|| (*Res == OMPDeclareTargetDeclAttr::MT_To && HasRequiresUnifiedSharedMemory)` and fix the message too


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1125
+  /// clause OR as declare target with to clause and unified memory.
+  virtual Address getAddrOfDeclareTargetClause(const VarDecl *VD);
 
----------------
Better to name it `getAddrOfDeclareTargetVar`


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2483-2485
+          else if (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+                   (*Res == OMPDeclareTargetDeclAttr::MT_To &&
+                    UnifiedMemoryEnabled))
----------------
Better to keep the assert but with a different condition.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63108