[PATCH] D124093: [PowerPC] Fixing implicit castings in altivec for -fno-lax-vector-conversions

[Nemanja Ivanovic via Phabricator via cfe-commits]

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