This is an archive of the discontinued LLVM Phabricator instance.

Implement vector_insert_exp builtins - clang portion
ClosedPublic

Authored by syzaara on Oct 25 2016, 11:41 AM.

Details

Summary

Add vec_insert_exp with prototypes:

vector double vec_insert_exp (vector double, vector unsigned long long);
vector double vec_insert_exp (vector unsigned long long, vector unsigned long long);
vector float vec_insert_exp (vector float, vector unsigned int);
vector float vec_insert_exp (vector unsigned int, vector unsigned int);

Diff Detail

Event Timeline

syzaara updated this revision to Diff 75747.Oct 25 2016, 11:41 AM
syzaara retitled this revision from to Implement vector_insert_exp builtins.
syzaara updated this object.
syzaara retitled this revision from Implement vector_insert_exp builtins to Implement vector_insert_exp builtins - clang portion.Oct 25 2016, 11:45 AM
amehsan edited edge metadata.Oct 25 2016, 12:31 PM

@syzaara @sfertile

When you create a patch, you need to subscribe llvm-commits as well. This is a mailing list. Unfortunately there is a phabricator bug, that if you do not add llvm-commits when you create the patch, an email will not be sent to it, when you subscribe it afterwards. So technically some people may ask you to create your patch again if llvm-commits is not subscribed. I leave it to @kbarton if he wants you to create the patch again. I subscribe llvm-commits to this patch so you are familiar with the ID for the future patches.

amehsan edited edge metadata.Oct 25 2016, 12:31 PM
amehsan added a subscriber: llvm-commits.

I realized @sfertile has that subscription on his patch :)

nemanjai edited edge metadata.Oct 25 2016, 1:00 PM

@syzaara @sfertile

When you create a patch, you need to subscribe llvm-commits as well. This is a mailing list. Unfortunately there is a phabricator bug, that if you do not add llvm-commits when you create the patch, an email will not be sent to it, when you subscribe it afterwards. So technically some people may ask you to create your patch again if llvm-commits is not subscribed. I leave it to @kbarton if he wants you to create the patch again. I subscribe llvm-commits to this patch so you are familiar with the ID for the future patches.

This applies to the LLVM portion of this patch. For the clang ones, we should subscribe cfe-commits. Thanks for pointing this out @amehsan. I haven't checked the patches from @jtony and @sfertile, but in any case we should be sure to subscribe the right "commits" list to our patches so that the community sees our patches.

@syzaara @sfertile

When you create a patch, you need to subscribe llvm-commits as well. This is a mailing list. Unfortunately there is a phabricator bug, that if you do not add llvm-commits when you create the patch, an email will not be sent to it, when you subscribe it afterwards. So technically some people may ask you to create your patch again if llvm-commits is not subscribed. I leave it to @kbarton if he wants you to create the patch again. I subscribe llvm-commits to this patch so you are familiar with the ID for the future patches.

This applies to the LLVM portion of this patch. For the clang ones, we should subscribe cfe-commits. Thanks for pointing this out @amehsan. I haven't checked the patches from @jtony and @sfertile, but in any case we should be sure to subscribe the right "commits" list to our patches so that the community sees our patches.

You are right :-)

nemanjai added inline comments.Oct 25 2016, 1:08 PM
lib/Headers/altivec.h
2500

Hmm... I think

&& __POWER9_VECTOR__
test/CodeGen/builtins-ppc-p9vector.c
704

I don't think this is guaranteed to succeed. If I remember correctly, Clang names temporaries differently with and without asserts. Maybe it wasn't asserts, but I remember something made Clang choose different names for temporaries (rather than %0, %1, etc.). So I think this should suffice as the regex pattern:
%{{.+}}

nemanjai added inline comments.Oct 25 2016, 1:11 PM
lib/Headers/altivec.h
2500

Actually, I take that back. It should only be POWER9_VECTOR.
This is important to get right because using this on a P8 or older machine will cause a run-time rather than a compile-time failure (i.e. it will compile just fine and executing the code would result in an illegal instruction).

echristo accepted this revision.Oct 25 2016, 1:20 PM
echristo added a reviewer: echristo.

Agreeing with Nemanja on a comment, once they're happy this LGTM.

-eric

lib/Headers/altivec.h
2500

+1

test/CodeGen/builtins-ppc-p9vector.c
704

Correct.

This revision is now accepted and ready to land.Oct 25 2016, 1:20 PM
Eugene.Zelenko added inline comments.
lib/Headers/altivec.h
2502

Please remove space between function name and open bracket. Same below.

syzaara updated this revision to Diff 75897.Oct 26 2016, 8:08 AM
syzaara edited edge metadata.

Fix formatting, remove #ifdef VSX, and update regex for test.

Committed revision 285229.
Zaara, please close this review when you get in if there are no buildbot failures in the meantime.

jtony closed this revision.Oct 27 2016, 7:40 AM