[Openmp-commits] [PATCH] D59783: [OpenMP] Implement 5.0 memory management

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: runtime/src/include/50/omp.h.var:294
+      omp_thread_mem_alloc = 8,
+      KMP_ALLOCATOR_MAX_HANDLE = UINTPTR_MAX
+    } omp_allocator_handle_t;
----------------
AndreyChurbanov wrote:
> ABataev wrote:
> > By default, the base type for enum is `int`. Value `UINTPTR_MAX` is too big for `int`.
> Alexey, theoretically you are correct.  But the majority of compilers (gcc, clang, icc, maybe others) support pointer-size enum by default.  The OpenMP language committee decided that this type must be enumeration in order to allow strict type checking by compilers. And at the same time it is supposed to keep pointer value as a convenient way to designate custom memory allocator or custom memory space object.
> 
> An alternative would require mapping of pointers to integers inside runtime library that looks a worse solution comparing to utilizing common non-standard language extension.
> 
> Compilers those do not support pointer-size enumeration can use implementation under #if above, that is currently specified for cl and icl (and maybe others) on Windows. This breaks OpenMP specification requirement, but again looks preferable comparing to mapping pointers to integers inside the runtime.
> 
> If you still think this is a big problem, then we can think of changing the OpenMP specification to not require enumeration types here, and allow integers instead.  But this is also not a simple and time consuming procedure.
I think it is better to use based enum explicitly, if the compiler supports it. I think you need to add a check for c++11 and use `enum omp_allocator_handle_t : uintptr_t` explicitly.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D59783