[PATCH] D16784: [OpenMP] Reorganize code to allow specialized code generation for different devices.

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks again for the review!


================
Comment at: lib/CodeGen/CGOpenMPRuntimeCommon.h:1
@@ +1,2 @@
+//===---- CGOpenMPRuntimeCommon.h - Helpers for OpenMP Runtimes Codegen ----==//
+//
----------------
ABataev wrote:
> I don't think we need this new file and new namespace. If some (currently) internal classes are needed, they must be exposed via CGOpenMPRuntime class. Derived class will be able to use these classes.
> Also, do not expose classes if they are not required right now.
Alright. I will add what is required as protected members of CGOpenMPRuntime. For now I didn't do that for any of the classes given that we are not doing any actual codegen.

================
Comment at: lib/CodeGen/CGOpenMPRuntimeCommon.h:267-268
@@ +266,4 @@
+
+LValue emitLoadOfPointerLValue(CodeGenFunction &CGF, Address PtrAddr,
+                                      QualType Ty);
+
----------------
ABataev wrote:
> I think it is better to make this function a member of CodeGenFunction.
Ok, once we start moving things to `CGOpenMPRntime` runtime, we add this to CodeGenFunction accordingly.

================
Comment at: lib/CodeGen/CGOpenMPRuntimeCommon.h:270-279
@@ +269,12 @@
+
+/// \brief Emits code for OpenMP 'if' clause using specified \a CodeGen
+/// function. Here is the logic:
+/// if (Cond) {
+///   ThenGen();
+/// } else {
+///   ElseGen();
+/// }
+void emitOMPIfClause(CodeGenFunction &CGF, const Expr *Cond,
+                            const RegionCodeGenTy &ThenGen,
+                            const RegionCodeGenTy &ElseGen);
+
----------------
ABataev wrote:
> If you need this one, make it a virtual member of CGOpenMPRuntime
Ok. I'll do  that once we post the codegen patches.

================
Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:24
@@ +23,3 @@
+  explicit CGOpenMPRuntimeNVPTX(CodeGenModule &CGM) : CGOpenMPRuntime(CGM) {
+    if (!CGM.getLangOpts().OpenMPIsDevice)
+      llvm_unreachable("OpenMP NVPTX is only prepared to deal with device code.");
----------------
ABataev wrote:
> I think it must be checked during creation of NVPTX specific OpenMPRuntime instance.
Ok, I did that. Also I added a new diagnostic in the compiler invocation so that the user gets a message instead of a stack dump. Let me know if you agree.


http://reviews.llvm.org/D16784