sfantao added a comment.
Hi Alexey,
Thanks for the review!
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:49-51
@@ -48,2 +48,5 @@
TargetRegion,
+ /// \brief Region that do not require function outlining and uses
+ /// information from a inner scope.
+ InlinedInnerRegion,
};
----------------
ABataev wrote:
> Do we really need this one? I don't think it will be used in codegen for
> directives, so do not add it.
Ok, I removed it. I was just following what was being done for the other APIs.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:316-318
@@ +315,5 @@
+ static bool classof(const CGCapturedStmtInfo *Info) {
+ return CGOpenMPRegionInfo::classof(Info) &&
+ cast<CGOpenMPRegionInfo>(Info)->getRegionKind() ==
+ InlinedInnerRegion;
+ }
----------------
ABataev wrote:
> I think it will be enough just to return 'false' here always, it should not
> be used in any casting operations ever
Ok, done.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4438-4440
@@ +4437,5 @@
+
+ if (NumTeams) {
+ assert(ThreadLimit && "Thread limit expression should be available along
"
+ "with number of teams.");
+ llvm::Value *OffloadingArgs[] = {
----------------
ABataev wrote:
> What if ThreadLimit is 'nullptr'? And why it cannot be 'nullptr' if NumTeams
> is not 'nullptr'?
Both values should be defined if there is a nested teams directive. If there
are no num_teams or thread_limit clauses (but we have a team directive), those
values will be defined with a int32 constant zero, which is the default value
for the runtime library. So, no matter the clauses, if there is a teams
directive both values will be defined. So, it is safe to assume that both
values will either be defined or both null.
I added a comment to clarify that.
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2712
@@ +2711,3 @@
+void CodeGenFunction::EmitOMPTeamsDirective(const OMPTeamsDirective &S) {
+ LexicalScope Scope(*this, S.getSourceRange());
+ const CapturedStmt &CS = *cast<CapturedStmt>(S.getAssociatedStmt());
----------------
ABataev wrote:
> Use 'OMPLexicalScope Scope(*this, S);' instead.
Done!
http://reviews.llvm.org/D17019
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
