[PATCH] [OPENMP] Codegen for 'if' clause

Alexey Bataev a.bataev at hotmail.com
Fri Oct 10 05:15:12 PDT 2014 

Hal, John, thanks for the review. I'll split this patch in two parts: one with some improvements and one with actual codegen for IfClause.

================
Comment at: include/clang/AST/StmtOpenMP.h:132
@@ +131,3 @@
+  /// \brief Gets single clause of the specified kind \a K associated with the
+  /// current directive iff there is only one clause of this kind.
+  const OMPClause *getSingleClause(OpenMPClauseKind K) const;
----------------
rjmccall wrote:
> hfinkel wrote:
> > Make sure this gets the improved comment text from D5145.
> This comment should clarify that it returns nil if there isn't a clause of that kind.  It only asserts that there aren't multiple clauses of that kind.
Ok, I replaced the comment and added additional comment that it returns nullptr if the directive does not have associated clause of the specified kind at all.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:134
@@ +133,3 @@
+  llvm::Value *ThreadID = nullptr;
+  OpenMPThreadIDMapTy::iterator I = OpenMPThreadIDMap.find(CGF.CurFn);
+  if (I != OpenMPThreadIDMap.end()) {
----------------
rjmccall wrote:
> It'd be nice to leave a comment here like "Check whether we've already cached a load of the thread id in this function."
I'll add one.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:141
@@ +140,3 @@
+    auto LVal = OMPRegionInfo->getThreadIDVariableLValue(CGF);
+    auto RVal = CGF.EmitLoadOfLValue(LVal, SourceLocation());
+    LVal = CGF.MakeNaturalAlignAddrLValue(RVal.getScalarVal(),
----------------
hfinkel wrote:
> Why not use Loc as the source location?
Ok, I'll fix it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:144
@@ +143,3 @@
+                                          ThreadIDVar->getType());
+    ThreadID = CGF.EmitLoadOfLValue(LVal, SourceLocation()).getScalarVal();
+    // If value loaded in entry block, use it everywhere in function.
----------------
hfinkel wrote:
> And here too.
> 
> I'm under the impression that it is better to have a bunch of code pointing to the same source location than to have code generated with no source locations.
Yes, will be done.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:145
@@ +144,3 @@
+    ThreadID = CGF.EmitLoadOfLValue(LVal, SourceLocation()).getScalarVal();
+    // If value loaded in entry block, use it everywhere in function.
+    if (CGF.Builder.GetInsertBlock() == CGF.AllocaInsertPt->getParent())
----------------
rjmccall wrote:
> Is there a reason we can't safely always load in the entry block?
Yes, actually this ThreadIdVar may be a function parameter. CodeGenFunction generates additional alloca for parameters and stores param to this generated new alloca. If currently we're not in EntryBlock I can't define where to put instructions with value loading, because I don't know where this store instruction. Loads must be dominated by this store, but I just can't guarantee it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:149
@@ -130,27 +148,3 @@
   } else {
-    // Check if current function is a function which has first parameter
-    // with type int32 and name ".global_tid.".
-    if (!CGF.CurFn->arg_empty() &&
-        CGF.CurFn->arg_begin()->getType()->isPointerTy() &&
-        CGF.CurFn->arg_begin()
-            ->getType()
-            ->getPointerElementType()
-            ->isIntegerTy() &&
-        CGF.CurFn->arg_begin()
-                ->getType()
-                ->getPointerElementType()
-                ->getIntegerBitWidth() == 32 &&
-        CGF.CurFn->arg_begin()->hasName() &&
-        CGF.CurFn->arg_begin()->getName() == ".global_tid.") {
-      CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
-      CGF.Builder.SetInsertPoint(CGF.AllocaInsertPt);
-      GTid = CGF.Builder.CreateLoad(CGF.CurFn->arg_begin());
-    } else {
-      // Generate "int32 .kmpc_global_thread_num.addr;"
-      CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
-      CGF.Builder.SetInsertPoint(CGF.AllocaInsertPt);
-      llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc)};
-      GTid = CGF.EmitRuntimeCall(
-          CreateRuntimeFunction(OMPRTL__kmpc_global_thread_num), Args);
-    }
-    OpenMPGtidMap[CGF.CurFn] = GTid;
+    // Generate "int32 .kmpc_global_thread_num.addr;"
+    CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
----------------
rjmccall wrote:
> This comment doesn't seem be accurate.  You're actually generating a call.
Agree, will be fixed.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:161
@@ -161,2 +160,3 @@
 void CGOpenMPRuntime::FunctionFinished(CodeGenFunction &CGF) {
   assert(CGF.CurFn && "No function in current CodeGenFunction.");
+  if (OpenMPThreadIDMap.count(CGF.CurFn))
----------------
rjmccall wrote:
> Would it make more sense for the CodeGenFunction to just have a lazily-allocated cache of arbitrary information associated with the OpenMPRuntime, rather than having tons of extra side-tables?
Agree, I'll rework it for OpenMPLocMap and OpenMPThreadIDMap.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:241
@@ +240,3 @@
+  // Build calls:
+  // __kmpc_serialized_parallel(&Loc, GTid);
+  llvm::Value *SerArgs[] = {EmitOpenMPUpdateLocation(CGF, Loc), ThreadID};
----------------
rjmccall wrote:
> Please leave a blank line before the emission of each call so that these don't all run together.
> 
> <-  That is, put one here.  (Before the comment line.)
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:246
@@ +245,3 @@
+  CGF.EmitRuntimeCall(RTLFn, SerArgs);
+  // OutlinedFn(&GTid, &zero, CapturedStruct);
+  auto ThreadIDAddr = EmitThreadIDAddress(CGF, Loc);
----------------
rjmccall wrote:
> <- And here.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:254
@@ +253,3 @@
+  CGF.EmitCallOrInvoke(OutlinedFn, OutlinedFnArgs);
+  // __kmpc_end_serialized_parallel(&Loc, GTid);
+  llvm::Value *EndSerArgs[] = {EmitOpenMPUpdateLocation(CGF, Loc), ThreadID};
----------------
rjmccall wrote:
> <- And here.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:264
@@ +263,3 @@
+// as "kmp_int32 *gtid");
+// otherwise, if we're not inside parallel region, but in regular serial code
+// region, get thread ID by calling kmp_int32 kmpc_global_thread_num(ident_t
----------------
hfinkel wrote:
> Capitalize O in otherwise. -- Move to previous line. 
Ok, I'll do.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:268
@@ +267,3 @@
+// temp.
+//
+llvm::Value *CGOpenMPRuntime::EmitThreadIDAddress(CodeGenFunction &CGF,
----------------
hfinkel wrote:
> Remove empty comment line.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:37
@@ +36,3 @@
+  /// the thread executing OpenMP construct. The type of this variable is
+  /// kmp_int32.
+  const VarDecl *getThreadIDVariable() const { return ThreadIDVar; }
----------------
rjmccall wrote:
> We don't necessarily need to do this right now, but eventually I think it makes sense to abstract this type so that it's private to the target runtime.
Ok, I'll move to CGOpenMPRuntime.cpp in a separate patch.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:160
@@ +159,3 @@
+  /// \brief Gets thread id value for the current thread.
+  /// \param CGF Reference to current CodeGenFunction.
+  /// \param Loc Clang source location.
----------------
rjmccall wrote:
> You don't need to comment CGF parameters; they're everywhere, and their purpose is obvious.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:161
@@ -144,1 +160,3 @@
+  /// \param CGF Reference to current CodeGenFunction.
+  /// \param Loc Clang source location.
   ///
----------------
rjmccall wrote:
> This kind of comment doesn't add anything that's not expressed in the type of the parameter.
> 
> A SourceLocation parameter to an Emit function has a conventional meaning in CodeGen, which is "here's a location to associate with this operation, for debug locations or whatever else".  You don't need to comment such parameters unless the SourceLocation's going to be used for some other purpose.
Got it

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:192
@@ -158,1 +191,3 @@
   /// \param Loc Clang source location.
+  /// \param OutlinedFn Outlined function to be run in parallel threads.
+  /// \param CapturedStruct A pointer to the record with the references to
----------------
rjmccall wrote:
> Please describe the expected type of the value.  It's something like void(*)(kmp_int32, struct context_vars*), right?
Almost. I'll add proper description of this function. Actually it returns void(*)(kmp_int32, kmp_int32, struct context_vars*)

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:26
@@ -25,1 +25,3 @@
 
+static void EmitOMPIfClause(CodeGenFunction &CGF, const Expr *Cond,
+                            std::function<void(bool)> CodeGen) {
----------------
rjmccall wrote:
> This deserves a comment, if just to explain what the parameter to the CodeGen callback means.
> 
> Also, please take the std::function as a const &.
Ok, will do.

http://reviews.llvm.org/D4716

More information about the cfe-commits mailing list