[PATCH] D43660: [OpenMP] Add OpenMP data sharing infrastructure using global memory

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:555
   Work.clear();
+  WrapperFunctionsMap.clear();
 
----------------
Do we need this?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:640-641
+      GlobalRecordSizeArg);
+  auto GlobalRecCastAddr = Bld.CreatePointerBitCastOrAddrSpaceCast(
+      GlobalRecValue, CGF.ConvertTypeForMem(RecTy)->getPointerTo());
+  FunctionToGlobalRecPtr.try_emplace(CGF.CurFn, GlobalRecValue);
----------------
`auto`->`llvm::Value *`


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:863-864
+    //   - the master thread ID;
+    emitCall(CGF, W, {Bld.getInt16(/*ParallelLevel=*/0),
+        getMasterThreadID(CGF)});
 
----------------
1. Update it to the latest  version. And use clang-format!
2. This must be in a separate patch


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:933
     /// void *outlined_function, int16_t IsOMPRuntimeInitialized);
-    llvm::Type *TypeParams[] = {CGM.Int8PtrTy,
-                                CGM.Int16Ty};
+    llvm::Type *TypeParams[] = {CGM.Int8PtrTy, CGM.Int16Ty};
     llvm::FunctionType *FnTy =
----------------
Separate patch


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1101
+    llvm::FunctionType *FnTy =
+        llvm::FunctionType::get(CGM.VoidTy, {}, /*isVarArg*/ false);
+    RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_end_sharing_variables");
----------------
`{}`->`llvm::None`


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1203-1210
+  auto *OutlinedFun = cast<llvm::Function>(
+      CGOpenMPRuntime::emitParallelOutlinedFunction(
+          D, ThreadIDVar, InnermostKind, CodeGen));
+  if (!isInSpmdExecutionMode()) {
+    llvm::Function *WrapperFun =
+        createDataSharingWrapper(OutlinedFun, D);
+    WrapperFunctionsMap[OutlinedFun] = WrapperFun;
----------------
Should it be in this patch?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.h:324-331
+  /// Emit function which wraps the outline parallel region
+  /// and controls the parameters which are passed to this function.
+  /// The wrapper ensures that the outlined function is called
+  /// with the correct arguments when data is shared.
+  llvm::Function *
+  createDataSharingWrapper(llvm::Function *OutlinedParallelFn,
+      const OMPExecutableDirective &D);
----------------
Do we need this function at all?


Repository:
  rC Clang

https://reviews.llvm.org/D43660