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

2016-12-19 Thread Ehsan Amiri via Phabricator via cfe-commits 
amehsan closed this revision.
amehsan added a comment.

https://reviews.llvm.org/rL287872


https://reviews.llvm.org/D26544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-11-18 Thread Kit Barton via cfe-commits 
kbarton accepted this revision.
kbarton added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D26544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-11-17 Thread Ehsan Amiri via cfe-commits 
amehsan updated the summary for this revision.
amehsan updated this revision to Diff 78376.

https://reviews.llvm.org/D26544

Files:
  lib/Headers/altivec.h
  test/CodeGen/builtins-ppc-altivec.c
  test/CodeGen/builtins-ppc-p8vector.c
  test/CodeGen/builtins-ppc-quadword.c
  test/CodeGen/builtins-ppc-vsx.c

Index: test/CodeGen/builtins-ppc-vsx.c
===
--- test/CodeGen/builtins-ppc-vsx.c
+++ test/CodeGen/builtins-ppc-vsx.c
@@ -69,6 +69,18 @@
 // CHECK: call <4 x float> @llvm.fabs.v4f32(<4 x float> %{{[0-9]*}})
 // CHECK-LE: call <4 x float> @llvm.fabs.v4f32(<4 x float> %{{[0-9]*}})
 
+  res_vd = vec_abs(vd);
+// CHECK: call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{[0-9]*}})
+// CHECK-LE: call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{[0-9]*}})
+
+  res_vf = vec_nabs(vf);
+// CHECK: [[VEC:%[0-9]+]] = call <4 x float> @llvm.fabs.v4f32(<4 x float> %{{[0-9]*}})
+// CHECK-NEXT: fsub <4 x float> , [[VEC]]
+
+  res_vd = vec_nabs(vd);
+// CHECK: [[VECD:%[0-9]+]] = call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{[0-9]*}})
+// CHECK: fsub <2 x double> , [[VECD]]
+
   dummy();
 // CHECK: call void @dummy()
 // CHECK-LE: call void @dummy()
@@ -1080,4 +1092,12 @@
 // CHECK: fmul <2 x double>
 // CHECK-LE: uitofp <2 x i64> %{{.*}} to <2 x double>
 // CHECK-LE: fmul <2 x double>
+
+  res_vf = vec_neg(vf);
+// CHECK: fsub <4 x float> , {{%[0-9]+}}
+// CHECK-LE: fsub <4 x float> , {{%[0-9]+}}
+
+  res_vd = vec_neg(vd);
+// CHECK: fsub <2 x double> , {{%[0-9]+}}
+// CHECK-LE: fsub <2 x double> , {{%[0-9]+}}
 }
Index: test/CodeGen/builtins-ppc-quadword.c
===
--- test/CodeGen/builtins-ppc-quadword.c
+++ test/CodeGen/builtins-ppc-quadword.c
@@ -119,11 +119,32 @@
 // CHECK-LE: @llvm.ppc.altivec.vsubeuqm
 // CHECK-PPC: error: assigning to '__vector __int128' (vector of 1 '__int128' value) from incompatible type 'int'
   
+  /* vec_sube */
+  res_vlll = vec_sube(vlll, vlll, vlll);
+// CHECK: @llvm.ppc.altivec.vsubeuqm
+// CHECK-LE: @llvm.ppc.altivec.vsubeuqm
+// CHECK-PPC: error: call to 'vec_sube' is ambiguous
+  
+  res_vulll = vec_sube(vulll, vulll, vulll);
+// CHECK: @llvm.ppc.altivec.vsubeuqm
+// CHECK-LE: @llvm.ppc.altivec.vsubeuqm
+// CHECK-PPC: error: call to 'vec_sube' is ambiguous
+  
+  res_vlll = vec_sube(vlll, vlll, vlll);
+// CHECK: @llvm.ppc.altivec.vsubeuqm
+// CHECK-LE: @llvm.ppc.altivec.vsubeuqm
+// CHECK-PPC: error: call to 'vec_sube' is ambiguous
+  
   res_vulll = vec_vsubeuqm(vulll, vulll, vulll);
 // CHECK: @llvm.ppc.altivec.vsubeuqm
 // CHECK-LE: @llvm.ppc.altivec.vsubeuqm
 // CHECK-PPC: error: assigning to '__vector unsigned __int128' (vector of 1 'unsigned __int128' value) from incompatible type 'int'
-  
+
+  res_vulll = vec_sube(vulll, vulll, vulll);
+// CHECK: @llvm.ppc.altivec.vsubeuqm
+// CHECK-LE: @llvm.ppc.altivec.vsubeuqm
+// CHECK-PPC: error: call to 'vec_sube' is ambiguous
+
   /* vec_subc */
   res_vlll = vec_subc(vlll, vlll);
 // CHECK: @llvm.ppc.altivec.vsubcuq
@@ -150,11 +171,21 @@
   res_vlll = vec_vsubecuq(vlll, vlll, vlll);
 // CHECK: @llvm.ppc.altivec.vsubecuq
 // CHECK-LE: @llvm.ppc.altivec.vsubecuq
-// CHECK-PPC: error: assigning to '__vector __int128' (vector of 1 '__int128' value) from incompatible type 'int'  
+// CHECK-PPC: error: assigning to '__vector __int128' (vector of 1 '__int128' value) from incompatible type 'int'
 
   res_vulll = vec_vsubecuq(vulll, vulll, vulll);
 // CHECK: @llvm.ppc.altivec.vsubecuq
 // CHECK-LE: @llvm.ppc.altivec.vsubecuq
+// CHECK-PPC: error: assigning to '__vector unsigned __int128' (vector of 1 'unsigned __int128' value) from incompatible type 'int'
+
+  res_vlll = vec_subec(vlll, vlll, vlll);
+// CHECK: @llvm.ppc.altivec.vsubecuq
+// CHECK-LE: @llvm.ppc.altivec.vsubecuq
+// CHECK-PPC: error: assigning to '__vector __int128' (vector of 1 '__int128' value) from incompatible type 'int'  
+
+  res_vulll = vec_subec(vulll, vulll, vulll);
+// CHECK: @llvm.ppc.altivec.vsubecuq
+// CHECK-LE: @llvm.ppc.altivec.vsubecuq
 // CHECK-PPC: error: assigning to '__vector unsigned __int128' (vector of 1 'unsigned __int128' value) from incompatible type 'int'  
 
 }
Index: test/CodeGen/builtins-ppc-p8vector.c
===
--- test/CodeGen/builtins-ppc-p8vector.c
+++ test/CodeGen/builtins-ppc-p8vector.c
@@ -73,13 +73,6 @@
 // CHECK-LE: call <2 x i64> @llvm.ppc.altivec.vmaxsd(<2 x i64> %{{[0-9]*}}, <2 x i64>
 // CHECK-PPC: error: call to 'vec_abs' is ambiguous
 
-  res_vd = vec_abs(vda);
-// CHECK: call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{.*}})
-// CHECK: store <2 x double> %{{.*}}, <2 x double>* @res_vd
-// CHECK-LE: call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{.*}})
-// CHECK-LE: store <2 x double> %{{.*}}, <2 x double>* @res_vd
-// CHECK-PPC: error: call to 'vec_abs' is ambiguous
-
   /* vec_add */
   res_vsll = vec_add(vsll, vsll);
 // CHECK: add <2 x i64>
@@

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

2016-11-16 Thread Ehsan Amiri via cfe-commits 
amehsan added inline comments.



Comment at: lib/Headers/altivec.h:350
+__tempc = __tempc & 0x0001;
+unsigned long long __longa = (unsigned long long) __tempa;
+unsigned long long __longb = (unsigned long long) __tempb;

kbarton wrote:
> 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]) & 
> > 0x;```
> 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, 
> 0x0001) and use it consistently. 
To come up with this code pattern I looked at the following pieces of codes:


```
unsigned long long f (int t) {
  return (unsigned long long)  t;
}
```
When compiled with optimization  produces 

```
define i64 @f(i32 signext %t) local_unnamed_addr #0 {
entry:
  %conv = sext i32 %t to i64
  ret i64 %conv
}

```
Which is incorrect. Also 


```
unsigned long long f (int t) {
  return (unsigned long long)(unsigned)  t;
}
~

```

results in 

```
define i64 @f(i32 signext %t) local_unnamed_addr #0 {
entry:
  %conv = zext i32 %t to i64
  ret i64 %conv
}

```

and 


```
.Lfunc_begin0:
# BB#0: # %entry
clrldi   3, 3, 32
blr

```

So I think the code here is optimal and correct and there is no need to change 
it.



Comment at: lib/Headers/altivec.h:10516
+ vector signed __int128 __c) {
+  return __builtin_altivec_vsubecuq(__a, __b, __c);
+}

kbarton wrote:
> Why do we mask the carries for sign/unsigned ints, but not __128 ints?
for quadword, hardware does the masking (implicitly, by only looking at the 
rightmost bit)


https://reviews.llvm.org/D26544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-11-15 Thread 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 & 0x0001;
+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]) & 
> 0x;```
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, 
0x0001) 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-11-13 Thread Nemanja Ivanovic via cfe-commits 
nemanjai added inline comments.



Comment at: lib/Headers/altivec.h:314
+  vector signed int __carry = __c & __mask;
+  return vec_add(vec_add(__a, __b), __mask);
+}

I don't understand why we're adding `__mask` to the sum of `__a` and `__b`. 
Shouldn't that be `__carry`?



Comment at: lib/Headers/altivec.h:322
+  vector unsigned int __carry = __c & __mask;
+  return vec_add(vec_add(__a, __b), __mask);
+}

Same comment as above.



Comment at: lib/Headers/altivec.h:349
+unsigned int __tempc = (unsigned int) __c[i];
+__tempc = __tempc & 0x0001;
+unsigned long long __longa = (unsigned long long) __tempa;

Is it not a little cleaner and more readable to just mask out the `__c` 
parameter before the loop (similarly to the masking you've done with `__mask` 
above)?



Comment at: lib/Headers/altivec.h:350
+__tempc = __tempc & 0x0001;
+unsigned long long __longa = (unsigned long long) __tempa;
+unsigned long long __longb = (unsigned long long) __tempb;

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]) & 0x;```


https://reviews.llvm.org/D26544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits