[PATCH] D10972: Add missing builtins to altivec.h for ABI compliance (vol. 3)

[Bill Schmidt]

wschmidt added a comment.

Hi Nemanja,

Looks pretty good.  I have a few comments to be dealt with before I can sign off.  Thanks!


================
Comment at: lib/Basic/Targets.cpp:1015
@@ -1014,2 +1014,3 @@
                                          DiagnosticsEngine &Diags) {
+  bool VSXDisabled = false;
   for (unsigned i = 0, e = Features.size(); i !=e; ++i) {
----------------
This way of handling the various -m arguments positionally is not quite right.  If both -mvsx and -mno-vsx are present in the command line, the last one seen wins.  That means that if the user specifies

  -mno-vsx -mpower8-vector -mvsx

we should turn on both the VSX and the P8Vector features.  The logic you are using will only turn on the VSX feature because the specification of -mpower8-vector happened to be in between -mno-vsx and -mvsx.

Also, it is an error to specify -mpower8-vector or -mdirect-moves if -mno-vsx is the switch value that wins due to occurring last.

So I suggest that you track each feature separately during this loop, obtaining individual values for HasVSX, HasP8Vector, and HasDirectMove.  Following the loop, if !HasVSX && (HasP8Vector || HasDirectMove), report an error and fail the compilation.

================
Comment at: lib/Headers/altivec.h:6687
@@ -6431,2 +6686,3 @@
+                                                   unsigned const int __b) {
   return vec_perm(__a, __a, (vector unsigned char)(__b));
 }
----------------
Missing the & 0x0F here.

================
Comment at: lib/Headers/altivec.h:7013
@@ -6712,2 +7012,3 @@
+  return (vector signed long long)__res;
 }
 
----------------
Seems like too much casting above.  It's ok to just use the original code here (and make "vector long long" into "vector unsigned long long").  The instruction will mask off all but the rightmost 6 bits of each element of b, so the signedness doesn't matter.

================
Comment at: test/CodeGen/builtins-ppc-p8vector.c:1010
@@ -706,2 +1009,3 @@
+// CHECK-LE: lshr <2 x i64>
 // CHECK-PPC: error: call to 'vec_sr' is ambiguous
 
----------------
This will probably change back per my earlier comment.


Repository:
  rL LLVM

http://reviews.llvm.org/D10972