Generally looks good. A couple organization suggestions to eliminate some of
the code duplication.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:426
@@ -425,1 +425,3 @@
}
+ case OMPRTL__kmpc_dispatch_init_4: {
+ auto ITy = CGM.Int32Ty;
----------------
Your only caller here pattern-matches on the integer size and signedness so
that it can pass the right enum value down just so that this code can
immediately switch out to cases that are identical except for slight
differences in integer size and signedness. This is kindof silly.
You don't *have* to shove everything through this one method; you can have a
CreateDispatchRuntimeFunction method or something that returns the appropriate
function from a named family for a given integer size and signedness. Or you
could be slightly less type-safe and just have CreateRuntimeFunction take
(defaultable) arguments that pick the right function from a family, like how
LLVM overloaded intrinsics work.
Please do something like that instead of having all this redundancy, though.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:383
@@ +382,3 @@
+ /// \param CGF Reference to current CodeGenFunction.
+ /// \param Loc Clang source location.
+ /// \param IVSize Size of the iteration variable in bits.
----------------
These are not useful documentation. You can just leave off the comment about
CGF. For Loc, there's probably an existing discussion about the purpose of the
source locations you can refer to.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:394
@@ +393,3 @@
+ /// returned.
+ virtual llvm::CallInst *EmitOMPForNext(CodeGenFunction &CGF,
+ SourceLocation Loc, unsigned IVSize,
----------------
Why does this specifically return a CallInst* instead of just a Value*?
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:591
@@ +590,3 @@
+ LB, UB, ST);
+ BoolCondVal = EmitScalarConversion(
+ Call, getContext().getIntTypeForBitwidth(32, /* Signed */ true),
----------------
Just have EmitOMPForNext do this conversion and return an i1.
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:610
@@ -564,1 +609,3 @@
+ if (Dynamic)
+ EmitIgnoredExpr(S.getInit());
----------------
Is there no init otherwise? Seems like something that should be asserted.
http://reviews.llvm.org/D7138
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
