Handle GPR and vector shift ops. Handle pmull also.

Handle GPR and vector shift ops. Handle pmull also.

For the integer shift ops, since the Src1 operand is forced
to be an immediate or register (cl), it should be legal to
have Dest+Src0 be either register or memory. However, we
are currently only using the register form. It might be the
case that shift w/ Dest+Src0 as mem are less optimized
on some micro-architectures though, since it has to load,
shift, and store all in one operation, but I'm not sure.

BUG=none
R=stichnot@chromium.org

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

[jvoung (off chromium), 2014-10-02 19:45:23]

jvoung@chromium.org changed reviewers:
+ stichnot@chromium.org

[Jim Stichnoth, 2014-10-02 20:35:17]

I didn't see any pmull tests?

I looked into the issue of shifting Dest/Src0 by a register or immediate
operand.  Our lowering style kind of limits that because of the x86 2-operand
instructions where Dest and Src0 are the same.  We would need to determine that
Dest and Src0's live ranges do not overlap, and then force them into the same
stack slot if they don't get register allocated.  Also, deal with one but not
the other getting a register, or both getting a different register for some
reason.

https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:582: bool ShiftHack = false;
const bool

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/8bit.pnacl.ll (right):

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/8bit.pnacl.ll:171: ; CHECK: shl {{[abcd]l|byte ptr}},
cl
Maybe [abd]l ?  Shouldn't be shifting cl by cl.

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:331: ; CHECK: rol
{{[abcd]x|word ptr}}, 8
Though it's very unlikely, you might want to allow bp, si, and di as well.

[jvoung (off chromium), 2014-10-02 22:04:13]

There is a test of vector mul in tests_lit/llvm2ice_tests/vector-arith.ll, which
CHECKs for "pmullw" and "pmulld" (d only for under SSE4.1):

mul <4 x i32> %arg0, %arg1

I added another one which loads from memory first, and for O2 the addressing
mode opts can help ensure one operand is memory. For Om1 it happens to not
register allocate and also loads from memory... Maybe I shouldn't assume that
though.

https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:582: bool ShiftHack = false;
On 2014/10/02 20:35:17, stichnot wrote:
> const bool

Done.

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/8bit.pnacl.ll (right):

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/8bit.pnacl.ll:171: ; CHECK: shl {{[abcd]l|byte ptr}},
cl
On 2014/10/02 20:35:17, stichnot wrote:
> Maybe [abd]l ?  Shouldn't be shifting cl by cl.

Ah right -- Done

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:331: ; CHECK: rol
{{[abcd]x|word ptr}}, 8
On 2014/10/02 20:35:17, stichnot wrote:
> Though it's very unlikely, you might want to allow bp, si, and di as well.

Done.

https://codereview.chromium.org/622113002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arith.ll (left):

https://codereview.chromium.org/622113002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arith.ll:24: ; RUN: %p2i -i %s --insts | %szdiff
%s | FileCheck --check-prefix=DUMP %s
Hmm p2i leaves out the inttoptr, so I wasn't sure how to work around that.

[jvoung (off chromium), 2014-10-02 22:09:53]

On 2014/10/02 20:35:17, stichnot wrote:
> I didn't see any pmull tests?
> 
> I looked into the issue of shifting Dest/Src0 by a register or immediate
> operand.  Our lowering style kind of limits that because of the x86 2-operand
> instructions where Dest and Src0 are the same.  We would need to determine
that
> Dest and Src0's live ranges do not overlap, and then force them into the same
> stack slot if they don't get register allocated.  Also, deal with one but not
> the other getting a register, or both getting a different register for some
> reason.

I guess I was imagining:

_mov(Dest, Src0)
// legalize Src1
_shl(Dest, Src1)

I see... so the problem is if Dest and Src0 are both mem/stack, then the _mov is
illegal.



> 
> https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h
> File src/IceInstX8632.h (right):
> 
>
https://codereview.chromium.org/622113002/diff/20001/src/IceInstX8632.h#newco...
> src/IceInstX8632.h:582: bool ShiftHack = false;
> const bool
> 
>
https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
> File tests_lit/llvm2ice_tests/8bit.pnacl.ll (right):
> 
>
https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
> tests_lit/llvm2ice_tests/8bit.pnacl.ll:171: ; CHECK: shl {{[abcd]l|byte ptr}},
> cl
> Maybe [abd]l ?  Shouldn't be shifting cl by cl.
> 
>
https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
> File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):
> 
>
https://codereview.chromium.org/622113002/diff/20001/tests_lit/llvm2ice_tests...
> tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:331: ; CHECK: rol
> {{[abcd]x|word ptr}}, 8
> Though it's very unlikely, you might want to allow bp, si, and di as well.

[Jim Stichnoth, 2014-10-03 03:10:44]

On 2014/10/02 20:35:17, stichnot wrote:
> I didn't see any pmull tests?

I meant, I expected to see tests of machine code bytes, like the new ones in
8bit.pnacl.ll.

[Jim Stichnoth, 2014-10-03 04:00:00]

lgtm

https://codereview.chromium.org/622113002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arith.ll (left):

https://codereview.chromium.org/622113002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arith.ll:24: ; RUN: %p2i -i %s --insts | %szdiff
%s | FileCheck --check-prefix=DUMP %s
On 2014/10/02 22:04:13, jvoung wrote:
> Hmm p2i leaves out the inttoptr, so I wasn't sure how to work around that.

Yeah, the DUMP test's days are numbered, as we move over to the direct bitcode
reader.

[jvoung (off chromium), 2014-10-03 18:12:15]

On 2014/10/03 03:10:44, stichnot wrote:
> On 2014/10/02 20:35:17, stichnot wrote:
> > I didn't see any pmull tests?
> 
> I meant, I expected to see tests of machine code bytes, like the new ones in
> 8bit.pnacl.ll.

Oh okay, I added a test that checks the machine code bytes. It might be better
if I had made some in-memory API to create the insts, then call the emit
functions and check the buffer, so that it's less dependent on the register
allocator, etc. =/

[jvoung (off chromium), 2014-10-03 18:17:20]

https://codereview.chromium.org/622113002/diff/100001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-arith.ll (right):

https://codereview.chromium.org/622113002/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-arith.ll:346: entry:
I moved this over to the address-mode-opt tests, which only tests O2.

[Jim Stichnoth, 2014-10-03 23:59:36]

still lgtm

[jvoung (off chromium), 2014-10-04 04:58:07]

Committed patchset #7 (id:120001) manually as
8bcca0418f0719b5acaa6cebcf9db20aa402aa2c (presubmit successful).