Lower bitmanip intrinsics, assuming absence of BMI/SSE4.2 for now.

Lower bitmanip intrinsics, assuming absence of BMI/SSE4.2 for now.

We'll need the fallbacks in any case. However, once we've
decided on how to specify the CPU features of the user
machine we can use the nicer LZCNT/TZCNT/POPCNT as well.

Adds cmov, bsf, and bsr instructions.

Calls a popcount helper function for machines without SSE4.2.

Not handling bswap yet (which can also take i16 params).

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3882
R=stichnot@chromium.org, wala@chromium.org

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

[Jim Stichnoth, 2014-07-14 23:20:45]

Initial pass, still need to look at the lit test.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
File crosstest/test_bitmanip.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip.cpp:1: //===- subzero/crosstest/test_bitmanip.cpp -
Implementation for tests --===//
Space it out to 80 cols :)

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:603: assert(Condition != Br_None);
Add an assert that Dest is a physical register.

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2445: //   cmovne DEST, IF_NOT_ZERO
Might want to document that the cmov instruction requires Dest to be a physical
register, hence the use of T_Dest.  I'm not sure, but this could be the first
instruction in Subzero where that is true and a specific register is not
required.

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2490: // TODO(jvoung): Determine if the user CPU
supports TZCNT (BMI).
Would it make sense to refactor this and lowerCtlz()?  It looks like there's a
lot in common.

[wala, 2014-07-15 00:16:32]

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
File crosstest/test_bitmanip.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip.cpp:11: // cross-testing. This uses a wrapper around the
GCC __builtins to access
Where were the GCC __builtins? I don't see them.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
File crosstest/test_bitmanip_main.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip_main.cpp:10: // Driver for cross testing atomic
intrinsics, via the sync builtins.
Incorrect description.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip_main.cpp:17: #include <pthread.h>
Is <pthread.h> ever used (also <cstdlib> below)?

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: class InstX8632BitScan : public InstX8632 {
The other instruction classes in the file are defined based on their structural
similarities, not their semantic similarities (i.e., we have InstX8632Binop,
InstX8632Ternop, and then "everything else"). This looks like it should be
InstX8632Unary. Sqrtss also fits the pattern for a unary instruction.

[Jim Stichnoth, 2014-07-15 17:15:02]

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: class InstX8632BitScan : public InstX8632 {
On 2014/07/15 00:16:31, wala wrote:
> The other instruction classes in the file are defined based on their
structural
> similarities, not their semantic similarities (i.e., we have InstX8632Binop,
> InstX8632Ternop, and then "everything else"). This looks like it should be
> InstX8632Unary. Sqrtss also fits the pattern for a unary instruction.

You're right that this is a unary op, but not the same as the currently defined
InstX8632Unaryop, where Dest and Src(0) are always the same (and Dest can be a
memory operand).

Maybe Unaryop should be redefined to fit bsf/bsr/sqrtss and then neg is its own
special instruction (since it's currently the only instance of a Unaryop).

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:269: ; CHECKO2REM-LABEL:
test_ctlz_32_ignored
It took me some time to remember what the O2REM test was for (testing for the
successful removal/elision of operations under O2).  Maybe add a comment near
the RUN line?

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:279: ; TODO(jvoung): If we
detect that LZCNT is supported.
The todo comment is a little odd...

[jvoung (off chromium), 2014-07-15 21:30:23]

Thanks!

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
File crosstest/test_bitmanip.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip.cpp:1: //===- subzero/crosstest/test_bitmanip.cpp -
Implementation for tests --===//
On 2014/07/14 23:20:45, stichnot wrote:
> Space it out to 80 cols :)

Done.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip.cpp:11: // cross-testing. This uses a wrapper around the
GCC __builtins to access
On 2014/07/15 00:16:31, wala wrote:
> Where were the GCC __builtins? I don't see them.

These are only using the wrappers, but the builtins would have been
__builtin_popcount{,ll},

I guess I might as well just say these use wrappers around the NaCl intrinsics.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
File crosstest/test_bitmanip_main.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip_main.cpp:10: // Driver for cross testing atomic
intrinsics, via the sync builtins.
On 2014/07/15 00:16:31, wala wrote:
> Incorrect description.

Oops copy-paste! Done.

https://codereview.chromium.org/390443005/diff/100001/crosstest/test_bitmanip...
crosstest/test_bitmanip_main.cpp:17: #include <pthread.h>
On 2014/07/15 00:16:31, wala wrote:
> Is <pthread.h> ever used (also <cstdlib> below)?

Done.

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:603: assert(Condition != Br_None);
On 2014/07/14 23:20:45, stichnot wrote:
> Add an assert that Dest is a physical register.

Done.

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: class InstX8632BitScan : public InstX8632 {
On 2014/07/15 17:15:02, stichnot wrote:
> On 2014/07/15 00:16:31, wala wrote:
> > The other instruction classes in the file are defined based on their
> structural
> > similarities, not their semantic similarities (i.e., we have InstX8632Binop,
> > InstX8632Ternop, and then "everything else"). This looks like it should be
> > InstX8632Unary. Sqrtss also fits the pattern for a unary instruction.
> 
> You're right that this is a unary op, but not the same as the currently
defined
> InstX8632Unaryop, where Dest and Src(0) are always the same (and Dest can be a
> memory operand).
> 
> Maybe Unaryop should be redefined to fit bsf/bsr/sqrtss and then neg is its
own
> special instruction (since it's currently the only instance of a Unaryop).

That sounds reasonable. I'll refactor.

I think movsx and movzx were also Unaryop-like.

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2445: //   cmovne DEST, IF_NOT_ZERO
On 2014/07/14 23:20:45, stichnot wrote:
> Might want to document that the cmov instruction requires Dest to be a
physical
> register, hence the use of T_Dest.  I'm not sure, but this could be the first
> instruction in Subzero where that is true and a specific register is not
> required.

Done.

I think the cvt instructions and some/most of the xmm ones, also require dest to
be a register?

Actually, looks like bsf and bsr also require the dest to be a register.

https://codereview.chromium.org/390443005/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2490: // TODO(jvoung): Determine if the user CPU
supports TZCNT (BMI).
On 2014/07/14 23:20:45, stichnot wrote:
> Would it make sense to refactor this and lowerCtlz()?  It looks like there's a
> lot in common.

Hmm, I tried that in the new patch set.

There are still a bunch of if-else because of the xor vs no xor, and the
different constants.  Otherwise, I might be able to remove one more if-else by
passing in the _bsf/_bsr method pointer.

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:269: ; CHECKO2REM-LABEL:
test_ctlz_32_ignored
On 2014/07/15 17:15:02, stichnot wrote:
> It took me some time to remember what the O2REM test was for (testing for the
> successful removal/elision of operations under O2).  Maybe add a comment near
> the RUN line?

Done.

https://codereview.chromium.org/390443005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:279: ; TODO(jvoung): If we
detect that LZCNT is supported.
On 2014/07/15 17:15:02, stichnot wrote:
> The todo comment is a little odd...

Uh hmm.. I think I'll just remove it. There's already a TODO above about LZCNT
for the 32-bit case.

[wala, 2014-07-15 21:51:14]

lgtm

[Jim Stichnoth, 2014-07-15 22:08:10]

LGTM with a couple nits.

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2117: InstCall *Call =
makeHelperCall(Val->getType() == IceType_i32 ?
Here and below, I'd prefer to make type comparison logic based on comparisons to
IceType_i64 instead of IceType_i32, which make it easier to identify the 64-bit
lowering special cases that may no longer be necessary for x86-64.  There are
currently some IceType_i32 comparisons, but usually to distinguish i32 from
i8/16 e.g. for mov versus movsx.

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2146: lowerCountZeros(IsCttz, Val->getType(),
Instr->getDest(), FirstVal, SecondVal);
80-col

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2162: lowerCountZeros(IsCttz, Val->getType(),
Instr->getDest(), FirstVal, SecondVal);
80-col

[jvoung (off chromium), 2014-07-15 23:10:33]

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2117: InstCall *Call =
makeHelperCall(Val->getType() == IceType_i32 ?
On 2014/07/15 22:08:09, stichnot wrote:
> Here and below, I'd prefer to make type comparison logic based on comparisons
to
> IceType_i64 instead of IceType_i32, which make it easier to identify the
64-bit
> lowering special cases that may no longer be necessary for x86-64.  There are
> currently some IceType_i32 comparisons, but usually to distinguish i32 from
> i8/16 e.g. for mov versus movsx.

Done.

Yep, would be good to identify 64-bit special cases.  As we discussed, for this
case, for X86-64 we'll still should still decide between the different helper
functions, but below there were some cases of deciding hi/lo operand that would
be different for x86-64.

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2146: lowerCountZeros(IsCttz, Val->getType(),
Instr->getDest(), FirstVal, SecondVal);
On 2014/07/15 22:08:09, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/390443005/diff/160001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2162: lowerCountZeros(IsCttz, Val->getType(),
Instr->getDest(), FirstVal, SecondVal);
On 2014/07/15 22:08:09, stichnot wrote:
> 80-col

Done.

[jvoung (off chromium), 2014-07-16 00:52:45]

Committed patchset #8 manually as re4da26f (presubmit successful).