[PATCH] D26304: [Power9] vector load/store with length - clang portion

[Nemanja Ivanovic via cfe-commits]

nemanjai added a comment.

Zaara, prior to committing this patch, can you write a test case that will actually use these builtins in an executable test case and confirm the results. We'll then run it on the simulator to ensure functional correctness.



================
Comment at: lib/Headers/altivec.h:38
 #define __ATTRS_o_ai __attribute__((__overloadable__, __always_inline__))
+#include <stddef.h>
 
----------------
I think size_t is only used for the load/store with length which are Power9 specific. I think we should guard this include with that macro as well to avoid changing behaviour of existing code inadvertently.


================
Comment at: lib/Headers/altivec.h:2609
+vec_xl_len(signed char * __a, size_t __b) {
+  return (vector signed char)__builtin_vsx_lxvl(__a, (__b << 56));
+}
----------------
This is undefined behaviour in 32-bit mode. Please guard for 64-bit mode unless this is already in such a block.


================
Comment at: test/CodeGen/builtins-ppc-p9vector.c:29
 
+float af[4] = {23.4f, 56.7f, 89.0f, 12.3f};
+double ad[2] = {23.4, 56.7};
----------------
I don't think we need to initialize these. And if we keep the initialization, let's use the same formatting (i.e. left curly brace, space, list of initializers, space, right curly brace).


================
Comment at: test/CodeGen/builtins-ppc-p9vector.c:35
+                          0,  1,  2,  3,  4,  5,  6,  7};
+signed short ass[8] = { -1, 2, -3, 4, -5, 6, -7, 8 };
+unsigned short aus[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };
----------------
Please select a naming scheme that does not lead to such unfortunate variable names.


https://reviews.llvm.org/D26304