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

Samuel Antao via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 2 22:45:00 PST 2016

sfantao added a comment.

Hi Alexey,

Thanks for the review!

Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:49-51
@@ -48,2 +48,5 @@
+    /// \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.


More information about the cfe-commits mailing list