John, thanks for the review!
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:118
@@ +117,3 @@
+ auto KmpRoutineEntryPointerQTy = C.getPointerType(KmpRoutineEntryTy);
+ KmpRoutineEntryPtrTy = CGM.getTypes().ConvertType(KmpRoutineEntryPointerQTy);
+ // Build struct kmp_task_t.
----------------
rjmccall wrote:
> Build this type lazily, please.
Ok
================
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");
----------------
rjmccall wrote:
> 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.
Ok
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:133
@@ -94,1 +132,3 @@
+ auto KmpTaskQTy = C.getRecordType(KmpTaskTRD);
+ KmpTaskTTy = CGM.getTypes().ConvertType(KmpTaskQTy);
}
----------------
rjmccall wrote:
> Build this type lazily, please.
Ok
================
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
----------------
rjmccall wrote:
> 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.
I'll fix it.
================
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).
----------------
rjmccall wrote:
> Missing braces.
My bad, will be fixed
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1012
@@ +1011,3 @@
+static RecordDecl *createNewKmpTaskTRecordDecl(ASTContext &C,
+ const RecordDecl *RD) {
+ auto *NewRD = C.buildImplicitRecord("kmp_task_t");
----------------
rjmccall wrote:
> 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.
Ok, I'll improve it
================
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
----------------
rjmccall wrote:
> Please add a newline after this comment to make it clear that it's describing
> the next sequence of steps as a whole.
Ok, will be done.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:17
@@ -16,2 +16,3 @@
+#include "clang/AST/Type.h"
#include "clang/Basic/OpenMPKinds.h"
----------------
rjmccall wrote:
> 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.
I'll fix it.
================
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.
----------------
rjmccall wrote:
> 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?
I'll try to split this function in two - one for tasks and another one for
parallel regions. I think this is better, rather than keep too complex single
function.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1124
@@ -1124,2 +1123,3 @@
case OMPD_task: {
+ QualType KmpInt32Ty = Context.getIntTypeForBitwidth(32, 1);
Sema::CapturedParamNameType Params[] = {
----------------
rjmccall wrote:
> This seems be hard-coding KMP runtime assumptions into Sema and the AST. Is
> that unavoidable?
Unfortunately, no. I'm trying to avoid runtime assumptions in common code, but
sometimes (like function parameters for outlined functions) I'm just forced to
make some hard-coding of runtime specific features.
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
