[PATCH] D101130: [PowerPC] Provide XL-compatible builtins in altivec.h

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.


================
Comment at: clang/lib/Headers/altivec.h:3055
 
+#ifdef __XL_COMPAT_ALTIVEC__
+/* vec_ctf */
----------------
jsji wrote:
> This only affects __VSX__ version right? If so, can we move `#ifdef __XL_COMPAT_ALTIVEC__` into `#ifdef __VSX__`？
> Or even better only into the part of affected types, eg: vector unsinged long long?
Sure, I'll make the non-vsx version unconditional on `__XL_COMPAT_ALTIVEC__`.


================
Comment at: clang/lib/Headers/altivec.h:3065
+                                                   (__b)),                     \
+             vector unsigned long long                                         \
+           : (__builtin_vsx_xvcvuxdsp((vector unsigned long long)(__a)) *      \
----------------
jsji wrote:
> jsji wrote:
> > Can we also add comments about what is the major difference with and without `__XL_COMPAT_ALTIVEC__` , for long term maintenance?
> > Can we also add comments about what is the major difference with and without `__XL_COMPAT_ALTIVEC__` , for long term maintenance?
> 
> I know they are in commit message, but I think comments in source code would be more helpful than in commit message. Thanks.
Will do.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat.c:2
+// REQUIRES: powerpc-registered-target
+// RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
+  // RUN:   -triple powerpc64-unknown-unknown -emit-llvm %s -o - \
----------------
cebowleratibm wrote:
> Suggest adding an explicit -mcpu.
I'm not sure that works with `cc1`. It would probably neeed to be something like `-target-cpu`.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat.c:6
+// RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx \
+  // RUN:   -triple powerpc64le-unknown-unknown -emit-llvm %s -o - \
+// RUN:   -D__XL_COMPAT_ALTIVEC__ | FileCheck %s
----------------
bmahjour wrote:
> [nit] remove extra space
Oops, I'll fix it.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat.c:8
+// RUN:   -D__XL_COMPAT_ALTIVEC__ | FileCheck %s
+#include <altivec.h>
+vector double vd = { 3.4e22, 1.8e-3 };
----------------
jsji wrote:
> Can we add a RUN line to test that novsx are not affected?
I don't think this is useful as that would be a duplicate of the tests in `builtins-ppc-vsx.c`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101130