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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits