[Openmp-commits] [PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via Openmp-commits]

peixin added a comment.

Except for three nits. LGTM.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);
----------------
Nit: integer -> 64-bit integer?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1676
+  Value *SrcLoc = getOrCreateIdent(getOrCreateSrcLocStr(DL));
+  Value *ThreadNum = getOrCreateThreadID(SrcLoc);
+  Constant *SchedulingType = ConstantInt::get(
----------------
peixin wrote:
> Can you move "Value *ThreadNum = getOrCreateThreadID(SrcLoc);" after "Builder.CreateStore(One, PStride);" in order that the "kmpc_global_thread_num" call is right before the "kmpc_static_init" call to keep consistence with others?
This comment is not addressed.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1765
+  switch (SchedKind) {
+  case llvm::omp::ScheduleKind ::OMP_SCHEDULE_Default:
+    assert(!ChunkSize && "No chunk size with default schedule (which for clang "
----------------
peixin wrote:
> Please remove the space between "ScheduleKind" and "OMP_SCHEDULE_Default"? Also for the following switch cases.
The extra space is not removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413