[Openmp-commits] [PATCH] Follow up to safe API patch

[Jonathan Peyton]

Hi hfinkel,

A while back, we made an initial change where dangerous C API functions were replaced with macros that translated the dangerous API function calls to safer function calls 
e.g., sprintf() replaced with KMP_SPRINTF() which translates to sprintf_s() on Windows.  Currently, the only operating system where this is applicable is Windows.  Unix-like systems are still using the dangerous API e.g., KMP_SPRINTF() translates to sprintf().  Hal had brought up a couple concerns over performance:
> +// _malloca was suggested, but it is not a drop-in replacement for _alloca
> +# define KMP_ALLOCA                  _alloca
> 1) "Is the only difference the fact that the size is capped at _ALLOCA_S_THRESHOLD? Would you hit this limit? Are there performance implications to making the change regardless?"
> -#define ngo_store_go(dst, src)   memcpy((dst), (src), CACHE_LINE)
> +#define ngo_store_go(dst, src)   KMP_MEMCPY((dst), (src), CACHE_LINE)
> 2) "The other changes look fine, but this could have performance implications, no? The compiler can optimize away the memcpy, but likely not the memcpy_s, and the associated validation is likely expensive. Should we avoid this here?"
For 1) Yes, it hits the limit, and the runtime should not allocate memory from the heap.  Currently performance is only affected on Windows because it is the only operating system using real safe API function calls.
For 2) That particular instance of memcpy only affects hierarchical barrier which is primarily used on Intel(R) MIC architectures.  For memcpy() in general, it currently is only affected on Windows and will not affect performance for Unix users.

If users are worried about performance, a CMake options can be added which always maps the macros to the unsafe C function calls.

REPOSITORY
  rL LLVM

http://reviews.llvm.org/D9918

Files:
  runtime/src/kmp_barrier.cpp
  runtime/src/kmp_safe_c_api.h

Index: runtime/src/kmp_barrier.cpp
===================================================================
--- runtime/src/kmp_barrier.cpp
+++ runtime/src/kmp_barrier.cpp
@@ -32,7 +32,7 @@
 #else
 #define ngo_load(src)            ((void)0)
 #define ngo_store_icvs(dst, src) copy_icvs((dst), (src))
-#define ngo_store_go(dst, src)   memcpy((dst), (src), CACHE_LINE)
+#define ngo_store_go(dst, src)   KMP_MEMCPY((dst), (src), CACHE_LINE)
 #define ngo_sync()               ((void)0)
 #endif /* KMP_MIC && USE_NGO_STORES */
 
Index: runtime/src/kmp_safe_c_api.h
===================================================================
--- runtime/src/kmp_safe_c_api.h
+++ runtime/src/kmp_safe_c_api.h
@@ -23,9 +23,7 @@
 # define RSIZE_MAX_STR ( 4UL << 10 ) // 4KB
 
 // _malloca was suggested, but it is not a drop-in replacement for _alloca
-// TODO: test performance and replace with _alloca (as below)
-# define KMP_ALLOCA                  alloca
-//# define KMP_ALLOCA                  _alloca
+# define KMP_ALLOCA                  _alloca
 
 # define KMP_MEMCPY_S                memcpy_s
 # define KMP_SNPRINTF                sprintf_s

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D9918.26265.patch
Type: text/x-patch
Size: 1128 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20150521/5ce7cf0d/attachment.bin>