[PATCH] D26544: [PPC] support for arithmetic builtins in the FE

[Kit Barton via cfe-commits]

kbarton added a comment.

There are several inline comments that need to be addressed.
I also think it's worthwhile putting a comment at the top of the review indicating the (assumed) semantics for the vec_sube and vec_subec instructions that are being implemented (i.e., the behaviour mimics the vec_subeuqm instructions and thus uses one's compliment plus the carry)



================
Comment at: lib/Headers/altivec.h:306
 }
+
 #endif
----------------
please remove blank line


================
Comment at: lib/Headers/altivec.h:350
+    __tempc = __tempc & 0x00000001;
+    unsigned long long __longa = (unsigned long long) __tempa;
+    unsigned long long __longb = (unsigned long long) __tempb;
----------------
nemanjai wrote:
> I think it's a little more clear and obvious what is happening if you actually have just a single cast and mask - i.e.
> ``` unsigned long long __longa = ((unsigned long long) __a[i]) & 0xFFFFFFFF;```
Is a mask actually needed? This seems to be what is done in the vec_addec function below, without the cast. I agree that is cleaner.

The other minor nit is to pick a single value for the mask (1, 0x01, 0x00000001) and use it consistently. 


================
Comment at: lib/Headers/altivec.h:10516
+             vector signed __int128 __c) {
+  return __builtin_altivec_vsubecuq(__a, __b, __c);
+}
----------------
Why do we mask the carries for sign/unsigned ints, but not __128 ints?


================
Comment at: lib/Headers/altivec.h:10524
+}
+
+
----------------
Please reorder these to put the __128 below signed and unsigned. 


================
Comment at: lib/Headers/altivec.h:10545
+  vector signed int __carry = __c & __mask;
+  return vec_add(vec_add(__a, ~__b), __carry);
+}
----------------
Is it possible to use vec_adde(__a, ~__b, __carry)?


https://reviews.llvm.org/D26544