Sorry for the delay.
In general, your code is very difficult to read; it doesn't use any vertical
whitespace within functions at all, so I can't easily understand how things are
supposed to be grouped.
The basic implementation seems fine, though I have a number of requests:
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:118
@@ +117,3 @@
+ auto KmpRoutineEntryPointerQTy = C.getPointerType(KmpRoutineEntryTy);
+ KmpRoutineEntryPtrTy = CGM.getTypes().ConvertType(KmpRoutineEntryPointerQTy);
+ // Build struct kmp_task_t.
----------------
Build this type lazily, please.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:119
@@ +118,3 @@
+ KmpRoutineEntryPtrTy = CGM.getTypes().ConvertType(KmpRoutineEntryPointerQTy);
+ // Build struct kmp_task_t.
+ auto *RD = C.buildImplicitRecord("kmp_task_t");
----------------
A better way of commenting the next few lines would be to just write the struct
definition out and then put all the addFieldToRecordDecl calls together.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:133
@@ -94,1 +132,3 @@
+ auto KmpTaskQTy = C.getRecordType(KmpTaskTRD);
+ KmpTaskTTy = CGM.getTypes().ConvertType(KmpTaskQTy);
}
----------------
Build this type lazily, please.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:252
@@ -210,4 +251,3 @@
auto RVal = CGF.EmitLoadOfLValue(LVal, Loc);
- LVal = CGF.MakeNaturalAlignAddrLValue(RVal.getScalarVal(),
- ThreadIDVar->getType());
- ThreadID = CGF.EmitLoadOfLValue(LVal, Loc).getScalarVal();
+ if (ThreadIDVar->getType()->isPointerType()) {
+ // Thread id is passed as a pointer
----------------
It looks like the invariants about getThreadIDVariable have changed; it used to
always be an int32*, and now it can be either that or an int32.
EmitThreadIDAddress appears to still assume that it's an ident*. You should
fix that and then add a test that the OpenMP constructs using that still work
correctly within a task.
You should also document this on CGOpenMPRegionInfo::ThreadIDVar.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:257
@@ +256,3 @@
+ ThreadID = CGF.EmitLoadOfLValue(LVal, Loc).getScalarVal();
+ } else
+ // Thread id is passed as a value (in tasks).
----------------
Missing braces.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1009
@@ +1008,3 @@
+};
+} // namespace
+
----------------
Somewhere around here would be a good place to put the code that lazily builds
KmpTaskTRD.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1012
@@ +1011,3 @@
+static RecordDecl *createNewKmpTaskTRecordDecl(ASTContext &C,
+ const RecordDecl *RD) {
+ auto *NewRD = C.buildImplicitRecord("kmp_task_t");
----------------
Comment, please. Please include the fact that this leaves the RecordDecl
incomplete.
Also, RD has to be KmpTaskTRD, so remove that argument and make this take the
CGOpenMPRuntime.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1093
@@ +1092,3 @@
+ // kmp_int32 flags, size_t sizeof_kmp_task_t, size_t sizeof_shareds,
+ // kmp_routine_entry_t *task_entry);
+ // Task flags. Format is taken from
----------------
Please add a newline after this comment to make it clear that it's describing
the next sequence of steps as a whole.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:17
@@ -16,2 +16,3 @@
+#include "clang/AST/Type.h"
#include "clang/Basic/OpenMPKinds.h"
----------------
I don't think you need this; you can just forward-declare QualType. If you do
need this, you can remove the forward-declaration of RecordDecl, I am positive
that Type.h does that already.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:288
@@ -261,1 +287,3 @@
+ /// /*ThreadID*/, kmp_int32 /*PartID*/, struct context_vars*) (if PartIdVar
!=
+ /// nullptr).
/// \param D OpenMP directive.
----------------
I was concerned about awkwardly making a single function do completely
different things based on weird defaulted parameter, but it turns out that
those types are exactly the same.
Would it be more appropriate to say that this creates an outlined function, and
that if PartID is given, it's bound to the appropriate parameter value?
Although it also looks like PartIDVar is completely ignored except for an
assertion. Is that expected to change?
================
Comment at: lib/Sema/SemaOpenMP.cpp:1124
@@ -1124,2 +1123,3 @@
case OMPD_task: {
+ QualType KmpInt32Ty = Context.getIntTypeForBitwidth(32, 1);
Sema::CapturedParamNameType Params[] = {
----------------
This seems be hard-coding KMP runtime assumptions into Sema and the AST. Is
that unavoidable?
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
