John, thanks for the review!
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:44
@@ -43,3 +43,3 @@
/// \brief Get an LValue for the current ThreadID variable.
- LValue getThreadIDVariableLValue(CodeGenFunction &CGF);
+ virtual LValue getThreadIDVariableLValue(CodeGenFunction &CGF);
----------------
rjmccall wrote:
> Okay. I think I see what you've done here with the subclass, so that this
> always produces a ident_t l-value, and thus its address is always an
> ident_t*. That makes a lot of sense to me. Please document that.
Ok, will do
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1138
@@ +1137,3 @@
+namespace {
+/// \brief Fields for type kmp_task_t.
+enum KmpTaskTFields {
----------------
rjmccall wrote:
> *Indexes* of fields in type kmp_task_t.
Ok, thanks.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1200
@@ +1199,3 @@
+ auto KmpInt32Ty = C.getIntTypeForBitwidth(/*DestWidth=*/32, /*Signed=*/1);
+ // Build type kmp_routine_entry_t (if not built yet.
+ if (!KmpRoutineEntryPtrTy) {
----------------
rjmccall wrote:
> Closing parenthesis. Also, I'd make a getter for this instead of inlining it
> here.
Ok, agree, that getter would look much better.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1230
@@ +1229,3 @@
+ auto *TaskEntry = llvm::Function::Create(
+ TaskEntryTy, /*Linkage=*/llvm::GlobalValue::InternalLinkage,
+ ".omp_task_entry.", &CGM.getModule());
----------------
rjmccall wrote:
> Argument comments are useful when somebody reading the code might not
> understand the purpose of the argument. In this case, that purpose is pretty
> obvious from the argument expression itself, so the comment is useless.
Ok, removed
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1266
@@ +1265,3 @@
+ KmpInt32Ty));
+ TaskEntryCGF.FinishFunction();
+
----------------
rjmccall wrote:
> It's generally better to emit helper functions in a separate function, so
> that any particular function only knows about one CodeGenFunction object:
> - It removes some code from what's already a pretty large function.
> - It makes the shared data between the two functions a lot more obvious.
> - It removes any confusion about which function any particular llvm::Value*
> is valid in, so that you aren't tempted to (e.g.) use SharedsParam in the CGF
> function instead of the TaskEntryCGF function.
Agree, I will create a separate function for proxy function
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1302
@@ +1301,3 @@
+ /*Volatile=*/false, CGM.PointerAlignInBytes, SharedsPtrTy, Loc),
+ Shareds, SharedsTy);
+ // TODO: generate function with destructors for privates.
----------------
rjmccall wrote:
> You're probably going to need to ask Sema to generate a copy constructor
> expression to get this right in C++. Feel free to do that in a separate
> patch.
I don't think I need to call a copy constructor here. Actually structure with a
list of shared variables is always POD type (it stores only pointers and
integer values), so we can simply copy it
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1318
@@ +1317,3 @@
+ getThreadID(CGF, Loc), NewTask};
+ CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__kmpc_omp_task), TaskArgs);
+}
----------------
rjmccall wrote:
> Nothing cares about the result of this call?
The check is required for untied tasks only, but they are not supported yet.
I'll add TODO here.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:273
@@ -264,1 +272,3 @@
+ /// This outlined function has type void(*)(kmp_int32 */*ThreadID*/,
kmp_int32
+ /// /*BoundID*/, struct context_vars*).
/// \param D OpenMP directive.
----------------
rjmccall wrote:
> I'd write these as parameter names, i.e. void (*)(kmp_int32 *ThreadID,
> kip_int32 BoundID, struct context_vars*). The C-style comment right next to
> the * is really hard to read.
Fixed
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:282
@@ +281,3 @@
+ /// outlined function has type void(*)(kmp_int32 /*ThreadID*/, kmp_int32
+ /// /*PartID*/, struct context_vars*).
+ /// \param D OpenMP directive.
----------------
rjmccall wrote:
> Same thing. Just write these comments as parameter names.
Fixed
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:737
@@ +736,3 @@
+ llvm::PointerIntPair<llvm::Value *, 1, bool> Final;
+ if (auto *Clause = S.getSingleClause(/*K=*/OMPC_final)) {
+ // If the condition constant folds and can be elided, try to avoid emitting
----------------
rjmccall wrote:
> You don't need to comment obvious arguments like this (and like a few other
> places nearby). The reader understands what the purpose of the argument is,
> and "K=" doesn't add anything meaningful anyway.
Ok, removed
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:746
@@ +745,3 @@
+ Final.setPointer(EvaluateExprAsBool(Cond));
+ } else
+ // By default the task is not final.
----------------
rjmccall wrote:
> Braces.
Fixed
http://reviews.llvm.org/D7560
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
