This is an archive of the discontinued LLVM Phabricator instance.

Add missing builtins to altivec.h for ABI compliance (vol. 3)
ClosedPublic

Authored by nemanjai on Jul 6 2015, 1:55 PM.

Details

Summary

This patch introduces a number of new vector interfaces to altivec.h.
In doing so, it touches on a number of issues that existed in existing
implementations as well as with how features are treated in the front end.
As a result, this patch accomplishes a few somewhat distinct but related
goals. Namely:

  1. Fixes how features and the related predefined macros are handled. Namely,

because -mvsx is implied by -mpower8-vector and by -mdirect-move. If we are on
a CPU that has those by default and we specify -mno-vsx, it turned off VSX in
the back end, but still left it on in the front end (most notably, the
predefined macro was still on).

  1. Added a number of new interfaces listed in the ABI
  2. Added a VSX path to one of the existing interfaces (vec_round)
  3. Changed the signatures of some existing functions to match the ABI
  4. Fixed how right shifts are generated as the implementation did not

match the ABI.

Changed signatures to conform to ABI:
vector short vec_perm(vector signed short, vector signed short, vector unsigned char)
vector int vec_perm(vector signed int, vector signed int, vector unsigned char)
vector long long vec_perm(vector signed long long, vector signed long long, vector unsigned char)
vector signed char vec_sld(vector signed char, vector signed char, const int)
vector unsigned char vec_sld(vector unsigned char, vector unsigned char, const int)
vector bool char vec_sld(vector bool char, vector bool char, const int)
vector unsigned short vec_sld(vector unsigned short, vector unsigned short, const int)
vector signed short vec_sld(vector signed short, vector signed short, const int)
vector signed int vec_sld(vector signed int, vector signed int, const int)
vector unsigned int vec_sld(vector unsigned int, vector unsigned int, const int)
vector float vec_sld(vector float, vector float, const int)
vector signed char vec_splat(vector signed char, const int)
vector unsigned char vec_splat(vector unsigned char, const int)
vector bool char vec_splat(vector bool char, const int)
vector signed short vec_splat(vector signed short, const int)
vector unsigned short vec_splat(vector unsigned short, const int)
vector bool short vec_splat(vector bool short, const int)
vector pixel vec_splat(vector pixel, const int)
vector signed int vec_splat(vector signed int, const int)
vector unsigned int vec_splat(vector unsigned int, const int)
vector bool int vec_splat(vector bool int, const int)
vector float vec_splat(vector float, const int)

Added a VSX path to:
vector float vec_round(vector float)

Added interfaces:
vector signed char vec_eqv(vector signed char, vector signed char)
vector signed char vec_eqv(vector bool char, vector signed char)
vector signed char vec_eqv(vector signed char, vector bool char)
vector unsigned char vec_eqv(vector unsigned char, vector unsigned char)
vector unsigned char vec_eqv(vector bool char, vector unsigned char)
vector unsigned char vec_eqv(vector unsigned char, vector bool char)
vector signed short vec_eqv(vector signed short, vector signed short)
vector signed short vec_eqv(vector bool short, vector signed short)
vector signed short vec_eqv(vector signed short, vector bool short)
vector unsigned short vec_eqv(vector unsigned short, vector unsigned short)
vector unsigned short vec_eqv(vector bool short, vector unsigned short)
vector unsigned short vec_eqv(vector unsigned short, vector bool short)
vector signed int vec_eqv(vector signed int, vector signed int)
vector signed int vec_eqv(vector bool int, vector signed int)
vector signed int vec_eqv(vector signed int, vector bool int)
vector unsigned int vec_eqv(vector unsigned int, vector unsigned int)
vector unsigned int vec_eqv(vector bool int, vector unsigned int)
vector unsigned int vec_eqv(vector unsigned int, vector bool int)
vector signed long long vec_eqv(vector signed long long, vector signed long long)
vector signed long long vec_eqv(vector bool long long, vector signed long long)
vector signed long long vec_eqv(vector signed long long, vector bool long long)
vector unsigned long long vec_eqv(vector unsigned long long, vector unsigned long long)
vector unsigned long long vec_eqv(vector bool long long, vector unsigned long long)
vector unsigned long long vec_eqv(vector unsigned long long, vector bool long long)
vector float vec_eqv(vector float, vector float)
vector float vec_eqv(vector bool int, vector float)
vector float vec_eqv(vector float, vector bool int)
vector double vec_eqv(vector double, vector double)
vector double vec_eqv(vector bool long long, vector double)
vector double vec_eqv(vector double, vector bool long long)
vector bool long long vec_perm(vector bool long long, vector bool long long, vector unsigned char)
vector double vec_round(vector double)
vector double vec_splat(vector double, const int)
vector bool long long vec_splat(vector bool long long, const int)
vector signed long long vec_splat(vector signed long long, const int)
vector unsigned long long vec_splat(vector unsigned long long,
vector bool int vec_sld(vector bool int, vector bool int, const int)
vector bool short vec_sld(vector bool short, vector bool short, const int)

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 29123.Jul 6 2015, 1:55 PM
nemanjai retitled this revision from to Add missing builtins to altivec.h for ABI compliance (vol. 3).
nemanjai updated this object.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: llvm-commits.
nemanjai added inline comments.Jul 6 2015, 2:03 PM
lib/Basic/Targets.cpp
1025

Oops, there was supposed to be a continue here. Added to the source tree.

wschmidt edited edge metadata.Jul 8 2015, 12:24 PM

Hi Nemanja,

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

lib/Basic/Targets.cpp
1015

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.

lib/Headers/altivec.h
6687

Missing the & 0x0F here.

7013

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.

test/CodeGen/builtins-ppc-p8vector.c
1010

This will probably change back per my earlier comment.

nemanjai marked 3 inline comments as done.Jul 9 2015, 6:27 AM
nemanjai added inline comments.
lib/Basic/Targets.cpp
1015

Thank you for this comment. I think I know what and how we need to do (this matches the gcc behaviour):

  • all three are set by default for ppc64le/pwr8
  • -mno-vsx disables all three defaults
  • if an -mno-vsx wins (appears after any -mvsx) and the user explicitly specifies one of the other two, a diagnostic is emitted
lib/Headers/altivec.h
6687

Greath catch, thank you.

7013

I am not sure if you are referring only to the vector long long overload of vec_sr or to all of them. In any case, the reason for the casts is that Clang will produce an lshr when the LHS is unsigned and an ashr when the LHS is signed. This is what was causing LLVM to emit vsr[bhwd] for the unsigned ones and vsra[bhwd] for the signed ones. So the casts are simply to ensure that the IR contains the lshr instructions rather than ashr/lshr depending on signedness.

Of course, I could have just defined builtins and called the same one for both signed and unsigned overloads, but I assumed that since IR instructions exist for this, it is better to retain the information about what operation is actually being performed in case the optimizer can use it.

test/CodeGen/builtins-ppc-p8vector.c
1010

If we indeed want logical shifts for vec_sr, then this should not change back.

wschmidt added inline comments.Jul 9 2015, 6:39 AM
lib/Basic/Targets.cpp
1015

Agreed, that is more complete than what I wrote.

lib/Headers/altivec.h
7013

You have wrongly changed the instruction to be generated. The user has chosen vec_sr, which means the user wants vsr[bhwd]. If they wanted vsra[bhwd], they could have chosen vec_sra. See figures 4-120 and 4-121 of http://www.freescale.com/files/32bit/doc/ref_manual/ALTIVECPIM.pdf to see the required instruction mappings.

wschmidt added inline comments.Jul 9 2015, 6:40 AM
lib/Headers/altivec.h
7013

Note that in this case we are adding code for the vector long long operands, which is not described in the PIM. But we should be consistent with existing behavior for the other types.

nemanjai updated this revision to Diff 29330.Jul 9 2015, 7:30 AM
nemanjai edited edge metadata.
nemanjai marked 2 inline comments as done.

Addressed the comments as follows:

  • Added the missing mask
  • Added handling for dependent feature options
  • No change to the vec_sr implementations, waiting for more clarification

What clarification are you waiting for?

wschmidt added inline comments.Jul 9 2015, 12:22 PM
lib/Headers/altivec.h
7013

OK, I take it back, I misread the code. We were already generating the wrong instruction here. Carry on... ;)

wschmidt accepted this revision.Jul 9 2015, 12:42 PM
wschmidt edited edge metadata.

LGTM with additional commentary to explain what's going on. :) Thanks.

lib/Basic/Targets.cpp
1340

After chatting with you on IRC, the above logic is correct but it is still pretty hard to understand. Please add a few more comments to explain the various scenarios.

This revision is now accepted and ready to land.Jul 9 2015, 12:42 PM

I would also like to see a test case or two for the confusing -mno-vsx -mpower8-vector stuff...

nemanjai closed this revision.Jul 10 2015, 6:12 AM

Committed revision 241904.