[PATCH] D29506: [OpenMP] Teams reduction on the NVPTX device.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

The patch is too big and quite hard to review? Could you split it into several smaller parts?



================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4280-4282
+  // We don't need debug information in this function as nothing here refers to
+  // user source code.
+  CGF.disableDebugInfo();
----------------
It is not quite so, at least we have a reference in a list of reductions in `reduction` clause, which may be considered a debug position


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:524
+
+  static bool classof(const CGOpenMPRuntime *RT) {
+    return RT->getKind() == RK_HOST;
----------------
arpith-jacob wrote:
> This is required to cast to the NVPTX runtime in a static function as follows;
> 
> CGOpenMPRuntimeNVPTX &RT = cast<CGOpenMPRuntimeNVPTX>(CGM.getOpenMPRuntime());
Are you going to make calls to `isa()`, `dyn_cast()` functions? If not, just use `static_cast<>()` instead.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:972-973
+  auto CastTy = Size <= 4 ? CGM.Int32Ty : CGM.Int64Ty;
+  auto *ElemCast = Bld.CreateSExtOrBitCast(Elem, CastTy);
+  auto *WarpSize = Bld.CreateTruncOrBitCast(getNVPTXWarpSize(CGF), CGM.Int16Ty);
+
----------------
I'd prefer you to use `CGF.EmitScalarConversion()` rather than Builder casts.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:975-978
+  llvm::SmallVector<llvm::Value *, 3> Args;
+  Args.push_back(ElemCast);
+  Args.push_back(Offset);
+  Args.push_back(WarpSize);
----------------
Do you really need a SmallVector<> or you can use just an array here?


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1140-1141
+    if (ShuffleInElement)
+      Elem = createRuntimeShuffleFunction(CGF, Private->getType(), Elem,
+                                          RemoteLaneOffset);
+
----------------
Enclose in braces


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1153-1156
+      CGF.EmitStoreOfScalar(Bld.CreatePointerBitCastOrAddrSpaceCast(
+                                DestElementAddr.getPointer(), CGF.VoidPtrTy),
+                            DestElementPtrAddr, /*Volatile=*/false,
+                            C.VoidPtrTy);
----------------
Braces


================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2249
+                                         ? OMPD_parallel_for_simd
+                                         : OMPD_parallel);
       // Emit post-update of the reduction variables if IsLastIter != 0.
----------------
OMPD_parallel or OMPD_parallel_for?


================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2424
     CGF.OMPCancelStack.emitExit(CGF, S.getDirectiveKind(), CodeGen);
-    CGF.EmitOMPReductionClauseFinal(S);
+    CGF.EmitOMPReductionClauseFinal(S, OMPD_parallel);
     // Emit post-update of the reduction variables if IsLastIter != 0.
----------------
OMPD_parallel_for?


https://reviews.llvm.org/D29506