[PATCH] D33053: [PowerPC] Implement vec_xxpermdi builtin.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

Add a test case like the one that currently crashes (see inline comment). Also, please do the following:

- Put a note in the description (and the commit message) with a link to the PR this fixes
- Put a note in the description that there is subsequent back end work to identify the correct shuffles (i.e. we currently won't emit an XXPERMDI when parameters are not vectors of doubleword elements)



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7997
 
-def err_shufflevector_non_vector : Error<
-  "first two arguments to __builtin_shufflevector must be vectors">;
----------------
It isn't appropriate to remove a target-independent diagnostic and replace it with a target-specific one (even if it's just in the name). I think it makes sense to update both `err_shufflevector_non_vector` and `err_shufflevector_incompatible_vector` to take an argument for the name of the builtin, but not to replace it with a target-specific one.

I would recommend renaming it to something like:
`err_shufflevector_non_vector` -> `err_vec_builtin_non_vector`
`err_shufflevector_incompatible_vector` -> `err_vec_builtin_incompatible_vector`
and add the parameter.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8005
 
+def err_vsx_builtin_nonconstant_argument3 : Error<
+  "third argument to %0 must be a constant integer between 0-3">;
----------------
We shouldn't diagnose out-of-range values.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8407
       // Create a shuffle mask of (1, 0)
-      Constant *ShuffleElts[2] = { ConstantInt::get(Int32Ty, 1),
-                                   ConstantInt::get(Int32Ty, 0)
-                                 };
+      Constant *ShuffleElts[2] = {ConstantInt::get(Int32Ty, 1),
+                                  ConstantInt::get(Int32Ty, 0)};
----------------
This change is unrelated and inconsequential. Please revert it.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8425
+    const int64_t MaxIndex = 3;
+    int64_t Index = clamp(ArgCI->getSExtValue(), 0, MaxIndex);
+    Ops[0] = Builder.CreateBitCast(Ops[0], llvm::VectorType::get(Int64Ty, 2));
----------------
We shouldn't clamp this. The correct semantics should be to mask out everything but the low order two bits (i.e. `&0x3`). But I think even that is unnecessary since you should just use the low order two bits from the input - see below.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8433
+    if (getTarget().isLittleEndian()) {
+      switch (Index) {
+      case 0:
----------------
The switch is overkill. You should just implement this in an obvious way (i.e. the same way as described in the ISA).
For big endian:
`ElemIdx0 = (Index & 2;) >> 1`
`ElemIdx1 = 2 + (Index & 1)`

For little endian:
`ElemIdx0 = (~Index & 1) + 2;`
`ElemIdx1 = ~Index & 2 >> 1;`

(of course, please verify the expressions).


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8482
+        Builder.CreateShuffleVector(Ops[0], Ops[1], ShuffleMask);
+    return ShuffleCall;
+  }
----------------
This is not going to work. You can try it with this simple test case that it crashes with:
```
#include <altivec.h>
vector int test(vector int a, vector int b) {
  return vec_xxpermdi(a, b, 0);
}
```
The problem is that the return type of the call expression may be different from the return type of the `shufflevector`.

You'll probably need something like this:
```
    QualType BIRetType = E->getType();
    auto RetTy = ConvertType(BIRetType);
    return Builder.CreateBitCast(ShuffleCall, RetTy);
```


================
Comment at: lib/Sema/SemaChecking.cpp:3900
+// vector short vec_xxsldwi(vector short, vector short, int);
+bool Sema::SemaBuiltinVSX(CallExpr *TheCall, unsigned NumArgs) {
+  if (TheCall->getNumArgs() < NumArgs)
----------------
I assume that we won't even need this at all if we're not diagnosing the range of the third argument.


================
Comment at: lib/Sema/SemaChecking.cpp:3915
+  llvm::APSInt Value;
+  bool IsICE = TheCall->getArg(2)->isIntegerConstantExpr(Value, Context);
+  if (!(IsICE && Value >= 0 && Value <= 3))
----------------
No, we don't want to diagnose this. Values larger than 3 should just use the low-order two bits. This matches GCC behaviour.


https://reviews.llvm.org/D33053