[PATCH] D28145: [OpenMP] Basic support for a parallel directive in a target region on an NVPTX device.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:114
   /// \brief Get the name of the capture helper.
-  StringRef getHelperName() const override { return ".omp_outlined."; }
+  StringRef getHelperName() const override { return "__omp_outlined__"; }
 
----------------
arpith-jacob wrote:
> On the nvptx device, it is illegal for an identifier to contain a dot ('.') so I've modified it here.  If there is a better way to do this, please let me know.
Could you just override this function in CGOpenMPRuntimeNVPTX?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:127
+/// Get thread id in team.
+/// FIXME: Remove the expensive remainder operation.
+static llvm::Value *getTeamThreadId(CodeGenFunction &CGF) {
----------------
I believe this FIXME is not needed as there is already no reminder operation.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:130
+  CGBuilderTy &Bld = CGF.Builder;
+  // N % M = N & (M-1) it M is a power of 2. The master Id is expected to be a
+  // power of two in all cases.
----------------
s/it/if/g?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:325-326
+    // Try to match this outlined function.
+    auto ID = Bld.CreatePtrToInt(W, CGM.Int64Ty);
+    ID = Bld.CreateIntToPtr(ID, CGM.Int8PtrTy);
+    llvm::Value *WorkFnMatch =
----------------
Why you need double casting, could you just use Bld.CreatePointerBitCastOrAddrSpaceCast()?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:339
+    // FIXME: Pass arguments to outlined function from master thread.
+    auto Fn = cast<llvm::Function>(W);
+    Address ZeroAddr =
----------------
auto *


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:343-345
+    llvm::SmallVector<llvm::Value *, 16> FnArgs;
+    FnArgs.push_back(ZeroAddr.getPointer());
+    FnArgs.push_back(ZeroAddr.getPointer());
----------------
Could you use just an array here instead of SmallVector?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:358
+      createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_kernel_end_parallel),
+      ArrayRef<llvm::Value *>());
   CGF.EmitBranch(BarrierBB);
----------------
Use llvm::None instead of default constructor.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:561
+    OutlinedFnArgs.push_back(
+        llvm::Constant::getNullValue(CGM.Int32Ty->getPointerTo()));
+    OutlinedFnArgs.push_back(
----------------
use llvm::ConstantPointerNull::get() instead.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:563
+    OutlinedFnArgs.push_back(
+        llvm::Constant::getNullValue(CGM.Int32Ty->getPointerTo()));
+    OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
----------------
use llvm::ConstantPointerNull::get() instead.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:573-575
+  if (IfCond) {
+    emitOMPIfClause(CGF, IfCond, L0ParallelGen, SeqGen);
+  } else {
----------------
No need for braces in one-line substatement


https://reviews.llvm.org/D28145