I agree that this is looking really good.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:49-64
@@ +48,18 @@
+ enum OpenMPLocationFlags {
+ /// \brief Use tramponline for internal microtask.
+ OMP_IDENT_IMD = 0x01,
+ /// \brief Use c-style ident structure.
+ OMP_IDENT_KMPC = 0x02,
+ /// \brief Atomic reduction option for kmpc_reduce.
+ OMP_ATOMIC_REDUCE = 0x10,
+ /// \brief Explicit 'barrier' directive.
+ OMP_IDENT_BARRIER_EXPL = 0x20,
+ /// \brief Implicit barrier in code.
+ OMP_IDENT_BARRIER_IMPL = 0x40,
+ /// \brief Implicit barrier in 'for' directive.
+ OMP_IDENT_BARRIER_IMPL_FOR = 0x40,
+ /// \brief Implicit barrier in 'sections' directive.
+ OMP_IDENT_BARRIER_IMPL_SECTIONS = 0xC0,
+ /// \brief Implicit barrier in 'single' directive.
+ OMP_IDENT_BARRIER_IMPL_SINGLE = 0x140
+ };
----------------
Our coding convention doesn't use ALL_CAPS here. These should probably be
OLF_IdentImd, OLF_IdentKmpc, ... to follow that convention (though I'm not
really sure what these names mean, and they don't seem to directly correspond
to the comments. What does IMD mean, or KMPC, for instance?)
Alternatively, you could explicitly state that these are named to match the
names from kmp.h, rather than just saying the descriptions are from there.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:49-50
@@ +48,4 @@
+ enum OpenMPLocationFlags {
+ /// \brief Use tramponline for internal microtask.
+ OMP_IDENT_IMD = 0x01,
+ /// \brief Use c-style ident structure.
----------------
Richard Smith wrote:
> Our coding convention doesn't use ALL_CAPS here. These should probably be
> OLF_IdentImd, OLF_IdentKmpc, ... to follow that convention (though I'm not
> really sure what these names mean, and they don't seem to directly correspond
> to the comments. What does IMD mean, or KMPC, for instance?)
>
> Alternatively, you could explicitly state that these are named to match the
> names from kmp.h, rather than just saying the descriptions are from there.
Typo of 'trampoline'.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:53-68
@@ +52,18 @@
+
+namespace {
+/// \brief RAII object that save current insert position and then restores it.
+class BuilderInsertPositionRAII {
+ CGBuilderTy &Builder;
+ CGBuilderTy::InsertPoint SavedIP;
+
+public:
+ BuilderInsertPositionRAII(CGBuilderTy &Builder,
+ llvm::Instruction *NewInsertPoint)
+ : Builder(Builder), SavedIP(Builder.saveIP()) {
+ assert(SavedIP.isSet() && "No insertion point is set!");
+ Builder.SetInsertPoint(NewInsertPoint);
+ }
+ ~BuilderInsertPositionRAII() { Builder.restoreIP(SavedIP); }
+};
+}
+
----------------
Looks like this can be replaced by `IRBuilder::InsertPointGuard`?
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:174
@@ +173,3 @@
+ RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_fork_call");
+ } break;
+ case OMPRTL__kmpc_global_thread_num: {
----------------
We put the `break;` inside the braces.
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:31-35
@@ +30,7 @@
+ CodeGenFunction CGF(CGM, true);
+ CGF.CapturedStmtInfo =
+ new CGCapturedStmtInfo(*CS, CS->getCapturedRegionKind());
+ llvm::Value *OutlinedFn = CGF.GenerateCapturedStmtFunction(
+ CS->getCapturedDecl(), CS->getCapturedRecordDecl(), CS->getLocStart());
+ delete CGF.CapturedStmtInfo;
+
----------------
Why are you creating the `CGCapturedStmtInfo` on the heap? Seems weird, since
it looks like you delete it two lines afterwards.
================
Comment at: lib/Sema/SemaOpenMP.cpp:673-680
@@ +672,10 @@
+ QualType KmpInt32PtrTy = Context.getPointerType(KmpInt32Ty);
+ Sema::ParamNameType Params[3] =
+ { std::make_pair("__global_tid", KmpInt32PtrTy),
+ std::make_pair("__bound_tid", KmpInt32PtrTy),
+ std::make_pair(StringRef(), QualType()) // __context with shared vars
+ };
+ ActOnCapturedRegionStart(Loc, CurScope, CR_OpenMP, Params);
+ }
+ break;
+ case OMPD_threadprivate:
----------------
Formatting here doesn't match our usual conventions. For the braced
initialization, put the `{` on the previous line, dedent the `};`. Dedent the
`}` for the `case`, and move the `break;` before it.
================
Comment at: lib/Sema/SemaOpenMP.cpp:685-686
@@ +684,4 @@
+ case OMPD_unknown:
+ case NUM_OPENMP_DIRECTIVES:
+ llvm_unreachable("Unknown OpenMP directive");
+ }
----------------
Yuck. `NUM_OPENMP_DIRECTIVES` shouldn't be a member of the enum, to remove the
need to do this.
================
Comment at: lib/Sema/TreeTransform.h:9784
@@ +9783,3 @@
+ SmallVector<Sema::ParamNameType, 4> Params;
+ for (unsigned i = 0; i < NumParams; ++i) {
+ if (i != ContextParamPos) {
----------------
Capital `I` please.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1080-1088
@@ -1079,5 +1079,11 @@
VisitDecl(CD);
+ unsigned ContextParamPos = Record[Idx++];
+ CD->setNothrow(Record[Idx++] != 0);
// Body is set by VisitCapturedStmt.
- for (unsigned i = 0; i < CD->NumParams; ++i)
- CD->setParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+ for (unsigned i = 0; i < CD->NumParams; ++i) {
+ if (i != ContextParamPos)
+ CD->setParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+ else
+ CD->setContextParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+ }
}
----------------
Do you have test coverage for this?
http://reviews.llvm.org/D2883
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
