[PATCH] D33236: [PowerPC] Implement vec_xxsldwi 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.

The changes required for this patch aren't too different from those required for the other patch. Just basically a bit more testing and making the code a bit more readable.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:8517
+    const int64_t MaxIndex = 3;
+    int64_t Index = clamp(ArgCI->getSExtValue(), 0, MaxIndex);
+    Ops[0] = Builder.CreateBitCast(Ops[0], llvm::VectorType::get(Int32Ty, 4));
----------------
We don't want to clamp this. We should just use the low order two bits. A simple
`int64_t Index = ArgCI->getSExtValue() & 0x3;`
Should suffice. (although it seems like unsigned and `getZExtValue()` should be used, but it doesn't really make a difference here).


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8529
+      case 0:
+      ElemIdx0 = 0;
+      ElemIdx1 = 1;
----------------
The indentation of the code in all the cases is off. It should be indented from the case label. In any case, I feel once again that a switch statement is a bit overkill and doesn't really provide additional insight into where the numbers came from.

For example, say you had expressions like this for the elements:
```
ElemIdx0 = (8 - Index) % 8;
ElemIdx1 = (9 - Index) % 8;
ElemIdx2 = (10 - Index) % 8;
ElemIdx3 = (11 - Index) % 8;
```
That is easy enough to explain in a comment such as:
```
// Little endian element N comes from element 8+N-Index of the 
// concatenated wide vector (of course, using modulo arithmetic on
// the total number of elements).
```


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8556
+    } else { // BigEndian
+      switch (Index) {
+      case 0:
----------------
Of course, the expressions to replace the switch here are rather obvious...
ElemIdx<N> = Index + N


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8593
+
+      Value *ShuffleCall = Builder.CreateShuffleVector(Ops[0], Ops[1],  ShuffleMask);
+      return ShuffleCall;
----------------
This will require the same type conversion as the other patch. I imagine it would crash on something like this:
```
vector double test(vector double a, vector double b) {
  return vec_xxsldwi(a, b, 1);
}
```

And of course, add that as a test case.


https://reviews.llvm.org/D33236