Subzero: Add fabs intrinsic support.

Subzero: Add fabs intrinsic support.

The intrinsic is lowered using the standard technique of masking off the FP sign bit, which is the high-order bit.

To construct this mask, we use the existing trick of loading a vector register with all "1" bits, then logical-shift-right by one bit.

In the future, we should add 128-bit vector values to the constant pool and force them to memory, and this could be used for the other routines that synthesize a vector constant.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4097
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=8c980d0de6592be36fae4ec8ae603dde7212c46c

[Jim Stichnoth, 2015-03-19 00:11:36]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com, mtrofin@chromium.org

[jvoung (off chromium), 2015-03-19 16:37:53]

Cool -- LGTM otherwise

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h#newcode811
src/IceInstX8632.h:811: assert(AllowAllTypes || Ty == IceType_v8i16 || Ty ==
IceType_v8i1 ||
We might be able to get away with just removing the assert at this layer, and
the lower level assembler code will know what types are handled:

E.g., for psra:


  if (Ty == IceType_i16) {
    EmitUint8(0xE1);
  } else {
    assert(Ty == IceType_i32);
  //...
  }

but what's missing is the check that isVectorType(Ty)

https://codereview.chromium.org/1022573004/diff/1/tests_lit/llvm2ice_tests/na...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/1022573004/diff/1/tests_lit/llvm2ice_tests/na...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:274: define float
@test_fabs_float(float %x, i32 %iptr) {
Probably don't need the iptr here or below if it's not used for a load?

https://codereview.chromium.org/1022573004/diff/1/tests_lit/reader_tests/nacl...
File tests_lit/reader_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/1022573004/diff/1/tests_lit/reader_tests/nacl...
tests_lit/reader_tests/nacl-other-intrinsics.ll:155: define float
@test_fabs_float(float %x, i32 %iptr) {
similar -- iptr not needed?

[Jim Stichnoth, 2015-03-19 18:35:15]

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h#newcode811
src/IceInstX8632.h:811: assert(AllowAllTypes || Ty == IceType_v8i16 || Ty ==
IceType_v8i1 ||
On 2015/03/19 16:37:53, jvoung wrote:
> We might be able to get away with just removing the assert at this layer, and
> the lower level assembler code will know what types are handled:
> 
> E.g., for psra:
> 
> 
>   if (Ty == IceType_i16) {
>     EmitUint8(0xE1);
>   } else {
>     assert(Ty == IceType_i32);
>   //...
>   }
> 
> but what's missing is the check that isVectorType(Ty)

The problem here is that psra's Ty argument is actually the element type (see 2
lines below this).  If Ty==IceType_i32, then ElementTy is also IceType_i32 which
psra will allow.

Maybe we could simplify this as a test for vector type, then let psll/psra/psrl
do the element-type check?

Except that psrl is explicitly used for fabs of scalar float and double so it
shouldn't be subject to an isVectorType check.

I made the isVectorType() simplification, ptal.

https://codereview.chromium.org/1022573004/diff/1/tests_lit/llvm2ice_tests/na...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/1022573004/diff/1/tests_lit/llvm2ice_tests/na...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:274: define float
@test_fabs_float(float %x, i32 %iptr) {
On 2015/03/19 16:37:53, jvoung wrote:
> Probably don't need the iptr here or below if it's not used for a load?

Done.

https://codereview.chromium.org/1022573004/diff/1/tests_lit/reader_tests/nacl...
File tests_lit/reader_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/1022573004/diff/1/tests_lit/reader_tests/nacl...
tests_lit/reader_tests/nacl-other-intrinsics.ll:155: define float
@test_fabs_float(float %x, i32 %iptr) {
On 2015/03/19 16:37:53, jvoung wrote:
> similar -- iptr not needed?

Done.

[jvoung (off chromium), 2015-03-19 18:50:34]

lgtm

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/1022573004/diff/1/src/IceInstX8632.h#newcode811
src/IceInstX8632.h:811: assert(AllowAllTypes || Ty == IceType_v8i16 || Ty ==
IceType_v8i1 ||
On 2015/03/19 18:35:15, stichnot wrote:
> On 2015/03/19 16:37:53, jvoung wrote:
> > We might be able to get away with just removing the assert at this layer,
and
> > the lower level assembler code will know what types are handled:
> > 
> > E.g., for psra:
> > 
> > 
> >   if (Ty == IceType_i16) {
> >     EmitUint8(0xE1);
> >   } else {
> >     assert(Ty == IceType_i32);
> >   //...
> >   }
> > 
> > but what's missing is the check that isVectorType(Ty)
> 
> The problem here is that psra's Ty argument is actually the element type (see
2
> lines below this).  If Ty==IceType_i32, then ElementTy is also IceType_i32
which
> psra will allow.
> 
> Maybe we could simplify this as a test for vector type, then let
psll/psra/psrl
> do the element-type check?
> 
> Except that psrl is explicitly used for fabs of scalar float and double so it
> shouldn't be subject to an isVectorType check.

Ah, ok that's what I missed.

> I made the isVectorType() simplification, ptal.

Okay this is a bit simpler / less specific to psra and psll.

[Jim Stichnoth, 2015-03-19 20:01:54]

Committed patchset #2 (id:20001) manually as
8c980d0de6592be36fae4ec8ae603dde7212c46c (presubmit successful).