[PATCH] D17019: [OpenMP] Code generation for teams - kernel launching

[Samuel Antao via cfe-commits]

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