[Openmp-commits] [PATCH] D84456: [OpenMP] Refactor memory allocation code for easier support for third party memory libraries

[Andrey Churbanov via Phabricator via Openmp-commits]

AndreyChurbanov added a comment.

>> The key difference is:
>> git diff   # gives you short patch
>> git diff -U999999  # gives you full patch
>>
>> The full patch should have complete source, as opposed to 3 lines above and below the changes for the short patch.
>
> Ugh, sorry I missed that! Fixed.

Not sure what do you mean by "fixed", I still don't see a context around the changes (probably you didn't upload full diff, even if you got it).
Anyway I tried to review the patch as it is. But please upload the full diff once you address the comments.



================
Comment at: openmp/runtime/src/kmp.h:923
 
+enum {
+  omp_ata_null = 0,
----------------
This enum is never used.  Better to remove the dead code.


================
Comment at: openmp/runtime/src/kmp_alloc.cpp:1236
+    .memspace = omp_default_mem_space,
+    .fb = omp_atv_abort_fb,
+    .alloc = kmp_default_alloc,
----------------
Why do you use abort_fb for all standard allocators?  The null_fb looks more logical to me for all but the omp_null_allocator.  Users can check the result and perform some actions if allocation fails.  Abort does not give them a chance.


================
Comment at: openmp/runtime/src/kmp_alloc.cpp:1356
+  if (kmp_destroy_allocator_p)
+    kmp_destroy_allocator_p(RCAST(kmp_allocator_t *, CCAST(omp_allocator_handle_t, allocator)));
+
----------------
CCAST is no-op here, please remove.  RCAST(kmp_allocator_t *, allocator) will work fine.


================
Comment at: openmp/runtime/src/kmp_alloc.cpp:1399
+  else if (allocator > kmp_max_mem_alloc)
+    al = RCAST(kmp_allocator_t *, CCAST(omp_allocator_handle_t, allocator));
+
----------------
CCAST is no-op here as well, please eliminate it.  This is historical artifact, as initially the parameter type was "const omp_allocator_handle_t". Now it is not const any more.


================
Comment at: openmp/runtime/src/kmp_alloc.cpp:1432
+	  return ptr;
     }
     }
----------------
Wrong intentation. Add 2 more spaces.


================
Comment at: openmp/runtime/src/kmp_alloc.cpp:1482
+
       if (al->pool_size > 0) { // custom allocator with pool size requested
         kmp_uint64 used =
----------------
Poor indentation. Please fix.


================
Comment at: openmp/runtime/src/kmp_settings.cpp:3312
     num = __kmp_str_to_int(buf, *next);
-    KMP_ASSERT(num > 0);
-    switch (num) {
-    case 4:
-      if (__kmp_memkind_available) {
-        __kmp_def_allocator = omp_high_bw_mem_alloc;
-      } else {
-        __kmp_msg(kmp_ms_warning,
-                  KMP_MSG(OmpNoAllocator, "omp_high_bw_mem_alloc"),
-                  __kmp_msg_null);
-        __kmp_def_allocator = omp_default_mem_alloc;
+    KMP_ASSERT(num > 0 && num < 9);
+  } else {
----------------
Either use
  sizeof(kmp_standard_allocators) / sizeof(kmp_standard_allocators[0])
instead of 9,or add some comment so that we don't miss this place in case number of standard allocators is changed in future.


================
Comment at: openmp/runtime/src/kmp_settings.cpp:3317
+    for(int i = 0; __kmp_allocator_names[i] != NULL; i++) {
+      if (__kmp_match_str("omp_high_bw_mem_alloc", buf, &next)) {
+        num = i + 1;
----------------
I'd guess __kmp_allocator_names[i] should be used instead of "omp_high_bw_mem_alloc" here.


================
Comment at: openmp/runtime/src/thirdparty/memkind/kmp_memkind.cpp:82
+
+    for(int i = 0; i < 9; i++) {
+      kmp_standard_allocators[0].alloc = kmp_memkind_alloc;
----------------
I think "sizeof(kmp_standard_allocators) / sizeof(kmp_standard_allocators[0])" is much safer than "9" as   upper bound.
As the number of standard allocators can be changed in future.


================
Comment at: openmp/runtime/src/thirdparty/memkind/kmp_memkind.cpp:91
+}
+#endif
+
----------------
#endif misplaced.  Move it inside __kmp_init_memkind function.


================
Comment at: openmp/runtime/src/thirdparty/memkind/kmp_memkind.cpp:96
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB
+  if (kmp_mk_check)
+    KE_TRACE(25, ("__kmp_fini_memkind: finalize memkind library\n"));
----------------
I'd remove this check and move the TRACE under if(h_memkind).

As theoretically kmp_mk_check can be non-NULL when h_memkind is NULL and memkind is not used (e.g. when expression in "if" on line 63 evaluates to false). Your code nullifies h_memkind, but does not nullify kmp_mk_check, kmp_mk_alloc, kmp_mk_free, mk_default, in case something goes wrong.


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

https://reviews.llvm.org/D84456