nemanjai added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15587
+
+ bool isUnaligned = (BuiltinID == PPC::BI__builtin_altivec_vinsw ||
+ BuiltinID == PPC::BI__builtin_altivec_vinsd)
----------------
This is strange. You don't need the ternary operator when you are simply
setting a `bool` variable. This is simply:
```
bool isUnaligned = (BuiltinID == PPC::BI__builtin_altivec_vinsw ||
BuiltinID == PPC::BI__builtin_altivec_vinsd);
```
Same below.
Also, please be careful of the naming convention (i.e. variables start with
upper case, functions with lowercase, etc.). I will not add further comments to
this end but please apply this change to the entire patch.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15604
+ // builtin called.
+ int validMaxValue = isUnaligned ? is32bit ? 12 : 8 : is32bit ? 3 : 1;
+
----------------
This is certainly concise, but trying to parse it puts my brain into an
infinite loop. Please write it as:
```
int ValidMaxValue = 0;
if (IsUnaligned)
ValidMaxValue = ... ? ... : ...
else
ValidMaxValue = ...
```
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15608
+ std::string rangeErrMsg = isUnaligned ? "byte" : "element";
+ rangeErrMsg += " number is outside of the valid range [0, ";
+ rangeErrMsg += llvm::to_string(validMaxValue) + "]";
----------------
Since you are building a string from pieces here, might as well mention the
value the user specified so it is something like:
`byte number 44 is outside of the valid range [0, 12]`
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15661
+ Value *Op1 = EmitScalarExpr(E->getArg(1));
+ llvm::Type *Int128Ty = llvm::IntegerType::get(getLLVMContext(), 128);
+ Ops.push_back(
----------------
Why not simplify the bitcasts below by just creating a pointer to the vector
type (call it say, `V1I128Ty`) here? Same in the next `case`.
================
Comment at: clang/lib/Headers/altivec.h:3211-3219
+ : (vector double)(__builtin_vsx_xvcvuxdsp(
\
+ (vector unsigned long long)(__a)) *
\
+ (vector float)(vector unsigned)((0x7f - (__b))
\
+ << 23)),
\
vector signed long long
\
- : (__builtin_vsx_xvcvsxdsp((vector signed long long)(__a)) *
\
- (vector float)(vector unsigned)((0x7f - (__b)) << 23)))
+ : (vector double)(__builtin_vsx_xvcvsxdsp(
\
+ (vector signed long long)(__a)) *
\
----------------
This is wrong. Please see the documentation for what the result type should be
under XL compat. I believe it needs to be `vector float` regardless of the
input.
================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c:8
// RUN: -D__XL_COMPAT_ALTIVEC__ -target-cpu pwr8 | FileCheck %s
-// RUN: %clang_cc1 -no-opaque-pointers -target-feature +altivec
-target-feature +vsx \
+// RUN: %clang_cc1 -flax-vector-conversions=none -no-opaque-pointers
-target-feature +altivec -target-feature +vsx \
// RUN: -triple powerpc64le-unknown-linux-gnu -emit-llvm %s -o - \
----------------
I don't know why only one of the functions below has checks for this run line,
but that needs to be fixed. Please add the `NOCOMPAT` checks on the other
functions.
================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c:30
// CHECK-NEXT: [[TMP1:%.*]] = call <4 x float> @llvm.ppc.vsx.xvcvsxdsp(<2 x
i64> [[TMP0]])
// CHECK-NEXT: fmul <4 x float> [[TMP1]], <float 6.250000e-02, float
6.250000e-02, float 6.250000e-02, float 6.250000e-02>
----------------
We compute a `vector float` but return a `vector double`. Something is wrong.
Please see my comment regarding this in `altivec.h`.
================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:746
[IntrNoMem, ImmArg<ArgIndex<2>>]>;
+
// P10 Vector Extract.
----------------
What happened here? Unrelated whitespace change. Please remove it before
committing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124093/new/
https://reviews.llvm.org/D124093
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
