[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.h:63
+/// Common pre(post)-action for different OpenMP constructs.
+class CommonActionTy final : public PrePostActionTy {
+  llvm::Value *EnterCallee;
----------------
I don't think it is good to name it CommonActionTy, I think it is something specific to NVPTX codegen. Could you give it some specific name + move it somewhere to NVPTX-related part of code (to .cpp file, if possible)


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:539-542
+    llvm::Value *EndArgs[] = {emitUpdateLocation(CGF, Loc), ThreadID};
+    CGF.EmitRuntimeCall(
+        createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_end_serialized_parallel),
+        EndArgs);
----------------
arpith-jacob wrote:
> ABataev wrote:
> > It is better to emit this code as PrePostAction, so it is called upon exit of any cleanup scope
> Alexey, do you mean clean up during the execution of the serialized parallel region?  Is something like this what you have in mind?  Thanks.
> 
>   auto &&SeqGen = [this, Fn, &CapturedVars, &RTLoc, &Loc](CodeGenFunction &CGF,
>                                                           PrePostActionTy &) {
>     auto &&CodeGen = [..](..) {
>         llvm::Value *Args[] = {RTLoc, ThreadID};
>         CGF.EmitRuntimeCall(
>             createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_serialized_parallel),
>             Args);
> 
>         llvm::SmallVector<llvm::Value *, 16> OutlinedFnArgs;
>         OutlinedFnArgs.push_back(
>             llvm::ConstantPointerNull::get(CGM.Int32Ty->getPointerTo()));
>         OutlinedFnArgs.push_back(
>             llvm::ConstantPointerNull::get(CGM.Int32Ty->getPointerTo()));
>         OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
>         CGF.EmitCallOrInvoke(Fn, OutlinedFnArgs);
>     };
> 
>     RegionCodeGenTy RCG(CodeGen);
>     CommonActionTy Action(
>         nullptr, llvm::None,
>         createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_end_serialized_parallel),
>         {emitUpdateLocation(CGF, Loc), ThreadID});
>     RCG.setAction(Action);
>     RCG(CGF);
>   };
> 
Yes, exactly.


================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:365
+    llvm::FunctionType *FnTy =
+        llvm::FunctionType::get(llvm::Type::getInt1Ty(CGM.getLLVMContext()),
+                                TypeParams, /*isVarArg*/ false);
----------------
Does it really return I1 type? Or I8?


https://reviews.llvm.org/D28145