[PATCH] D148916: [Support][Parallel] Initialize threadIndex and add assertion checking its usage.

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

> all above places are calls to mlir::failableParallelForEach, right? The llvm::parallel::parrallelFor is not used . Thus, it looks like llvm::parallel::parrallelFor may be changed ?

I haven't checked thoroughly but it does look like `mlir` has moved away from `llvm::parallel` and its `parallelFor` implementation supports nested parallel for loops.



================
Comment at: llvm/include/llvm/Support/Parallel.h:42-46
+  assert(((parallel::strategy.ThreadsRequested == 1) ||
+          (threadIndex != UINT_MAX)) &&
+         "getThreadIndex() must be called from the thread created by "
+         "ThreadPoolExecutor");
+  return threadIndex;
----------------
I tend to get a bad feeling when code is duplicated as this is in `Parallel.cpp`. I know macros aren't great either but perhaps define the "implementation" as a macro and use that here and in `Parallel.cpp`?


================
Comment at: llvm/lib/Support/Parallel.cpp:251-252
 
   for (; Begin != End; ++Begin)
     Fn(Begin);
 }
----------------
IIUC, this will be executed if `parallel::strategy.ThreadsRequested == 1` and in this case, `getThreadIndex()` will return `UINT_MAX`. Is this the intended behaviour?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148916