[libcxx-commits] [PATCH] D99836: [pstl] Implement OpenMP backend

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch! Please ping me once you've applied those comments and we should be close to ready to ship this.



================
Comment at: pstl/include/pstl/internal/omp/parallel_for.h:11
+
+#ifndef PSTL_INTERNAL_OMP_PARALLEL_FOR_H
+#define PSTL_INTERNAL_OMP_PARALLEL_FOR_H
----------------
rarutyun wrote:
> In all (or almost all) files the guard macro is not started from underscore. It's inconsistent with the rest of the code and makes somebody think that this macro is public.
> 
> It probably doesn't deserve one more patch but I would like to make sure it's properly merged to main branch.
Please fix. This should be `_PSTL_INTERNAL_OMP_PARALLEL_FOR_H` (and similarly throughout).


================
Comment at: pstl/include/pstl/internal/omp/parallel_for.h:14
+
+#include "util.h"
+
----------------
Missing `#include <cstddef>`


================
Comment at: pstl/include/pstl/internal/omp/parallel_for_each.h:27
+    // TODO: Think of an approach to remove the std::distance call
+    auto __size = ::std::distance(__first, __last);
+
----------------
It should be sufficient to use `std::distance`, not `::std::distance`. Any reason why you're doing the latter? Applies in a bunch of places.


================
Comment at: pstl/include/pstl/internal/omp/parallel_merge.h:92-94
+} // namespace __omp_backend
+} // namespace __pstl
+#endif // PSTL_INTERNAL_OMP_PARALLEL_MERGE_H
----------------
Apply throughout.


================
Comment at: pstl/include/pstl/internal/omp/util.h:28
+#if !defined(_OPENMP)
+#    error _OPENMP not defined.
+#endif
----------------
This error message isn't too helpful. Something like this would probably be better:

```
#if !defined(_OPENMP)
#    error The OpenMP PSTL backend requires OpenMP, but it looks like your platform doesn't implement it.
#endif
```

Is the above message accurate and what you're trying to convey to the user? Also, is there any way that you'd be able to include `<omp.h>` yet OpenMP wouldn't be available? If not, then the `#if !defined(_OPENMP)` block is useless altogether.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99836