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(>id, &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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
