[PATCH] [OPENMP] Codegen for the 'omp for' with static schedule (non-chunked).

[John McCall]

================
Comment at: include/clang/AST/StmtOpenMP.h:276
@@ -268,1 +275,3 @@
+    NextUpperBoundOffset = 15,
+    WorksharingArraysOffset = 16
   };
----------------
End with a comma, please.

Also, some of these enumerators (e.g. LastIterationOffset) clearly correspond to actual child expressions, and some of them (e.g. NonworksharingArraysOffset) do not.  Please:
- give the ArraysOffset enumerators a more distinctive name and
- put comments on both of them to indicate their special purpose.

Also, there should really be a comment somewhere in the class — probably right above this enum — describing the overall layout of the sub-expressions.  That comment should include the fact that there are all of these fixed children, but different numbers for different kinds, plus the three arrays of length CollapsedNum.

================
Comment at: include/clang/AST/StmtOpenMP.h:649
@@ -545,1 +648,3 @@
+         ArrayRef<Expr *> Counters, ArrayRef<Expr *> Updates,
+         ArrayRef<Expr *> Finals);
 
----------------
This is now officially Too Many Arguments.  Make a struct for passing them around, please.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:154
@@ -154,1 +153,3 @@
+  }
+  if (LocValue == nullptr) {
     // Generate "ident_t .kmpc_loc.addr;"
----------------
Is this a change in the invariants of OpenMPLocThreadIDMap?  That should be documented.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:537
@@ -443,1 +536,3 @@
 
+/// \brief Schedule types for 'omp for' loops (see enum sched_type in kmp.h).
+enum OpenMPSchedType {
----------------
This comment should be more explicit: these enumerators are taken from the enum sched_type in kmp.h.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:560
@@ +559,3 @@
+    Schedule = Chunked ? OMP_sch_static_chunked : OMP_sch_static;
+    break;
+  case OMPC_SCHEDULE_dynamic:
----------------
All of these cases should return immediately instead of modifying a local.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:577
@@ +576,3 @@
+  }
+  return Schedule;
+}
----------------
The switch should be exhaustive, and you should put an llvm_unreachable here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:600
@@ +599,3 @@
+  if (Chunk == nullptr)
+    Chunk = CGF.Builder.getIntN(IVSize, /*C*/ 1);
+  llvm::Value *Args[] = {
----------------
Please comment why this is reasonable.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:272
@@ +271,3 @@
+
+  /// \brief If the loop has static schedule, call
+  /// __kmpc_for_static_init(
----------------
The method assumes that the loop has static schedule.  You should probably change the method name to reflect that, and the comment should say "Given that a loop has static schedule..." rather than "If the loop has static schedule...".  Same thing for Fini, below.

Alternatively, if it's not supposed to assume that the loop has static schedule, there's no reason to document the exact call that it makes here, and you should describe what the method does at a higher level.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:296
@@ +295,3 @@
+  /// returned nesessary to generated the static_chunked scheduled loop.
+  /// \param Chunk Value of the chunk for the static_chunked scheduled loop.
+  ///
----------------
This comment should mention that nullptr has a special meaning.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:305
@@ +304,3 @@
+  /// \brief If the loop has static schedule, call
+  /// __kmpc_for_static_fini(ident_t *loc, kmp_int32 tid)
+  ///
----------------
Same comment as above: this function assumes that the loop has static schedule.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:312
@@ +311,3 @@
+  virtual void EmitOMPForFini(CodeGenFunction &CGF, SourceLocation Loc,
+                              OpenMPScheduleClauseKind ScheduleKind);
+
----------------
Please spell out Finish, even though it isn't spelled out in the runtime function name.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:473
@@ +472,3 @@
+  {
+    // Emit: if (LastIteration > 0) - begin.
+    RegionCounter Cnt = getPGORegionCounter(&S);
----------------
Describe this in higher-level terms.  "Skip the entire loop if we don't meet the precondition."  You're not hardcoding anything about what the precondition looks like here.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:528
@@ +527,3 @@
+      } else
+        llvm_unreachable("Requested OpenMP schedule is not yet implemented");
+    }
----------------
Don't crash.  CodeGenFunction has a perfectly good ErrorUnsupported method you can use.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:530
@@ +529,3 @@
+    }
+    // Emit: if (LastIteration != 0) - end.
+    EmitBranch(ContBlock);
----------------
Again, comment what you're actually doing.  Something like "We're now done with the loop, so jump to the continuation block."

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:545
@@ +544,3 @@
+
+  // Emit an implicit barrier at the end.
+  auto Flags = static_cast<CGOpenMPRuntime::OpenMPLocationFlags>(
----------------
Should this occur even if the precondition isn't met?

================
Comment at: lib/Sema/SemaOpenMP.cpp:2395
@@ -2394,1 +2394,3 @@
   Expr *Inc;
+  Expr *IL;
+  Expr *LB;
----------------
Yeah, see, this would be a great place to use that struct that includes all the expressions instead of spelling them all out again with terrible variable names.

================
Comment at: lib/Sema/SemaOpenMP.cpp:2858
@@ +2857,3 @@
+
+  // Increments for worksharing loops (LB = LB + ST; UB = UB + ST).
+  // Used for directives with static scheduling.
----------------
Every time you build up a bunch of expressions that you intend to emit as their own independent statement, you should be calling ActOnFinishFullExpr afterwards.  This is absolutely required if there's any potential of introducing C++ temporaries in these expressions.

http://reviews.llvm.org/D5865