Use three-address form of imul

Use three-address form of imul

Previously we did not take advantage of the three address versions of the
imul instruction.  With this we are able to avoid some copies before imuls.

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-09-25 00:51:19]

sehr@chromium.org changed reviewers:
+ jpp@chromium.org, stichnot@chromium.org

[sehr, 2015-09-25 00:51:34]

Three address imuls.  PTAL.

[Jim Stichnoth, 2015-09-25 21:02:10]

https://codereview.chromium.org/1365433004/diff/20001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1365433004/diff/20001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:754: void imul_imm(Type Ty, typename
Traits::GPRRegister dst,
I'm not adamant about this, but following the existing naming style in this
file, maybe these versions should be named imul() instead of imul_imm()?

(I don't have the same concern for the two ICE layers above this.)

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:1333: // The two-address version is used when
multiplying by a non-constant.
or for the 8-bit version (right?)

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:1350: Variable *Dest = this->getDest();
Add an assert that the type is i16 or i32.  (The emitIAS() method could also
have that assert, though the assembler already asserts that.)

[sehr, 2015-09-25 23:04:41]

PTAL.

https://codereview.chromium.org/1365433004/diff/20001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1365433004/diff/20001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:754: void imul_imm(Type Ty, typename
Traits::GPRRegister dst,
On 2015/09/25 21:02:10, stichnot wrote:
> I'm not adamant about this, but following the existing naming style in this
> file, maybe these versions should be named imul() instead of imul_imm()?
> 
> (I don't have the same concern for the two ICE layers above this.)

Done.

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:1333: // The two-address version is used when
multiplying by a non-constant.
On 2015/09/25 21:02:10, stichnot wrote:
> or for the 8-bit version (right?)

Done.

https://codereview.chromium.org/1365433004/diff/20001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:1350: Variable *Dest = this->getDest();
On 2015/09/25 21:02:10, stichnot wrote:
> Add an assert that the type is i16 or i32.  (The emitIAS() method could also
> have that assert, though the assembler already asserts that.)

Done.

[Jim Stichnoth, 2015-09-26 00:43:06]

lgtm

[sehr, 2015-10-06 17:27:02]

Committed patchset #5 (id:80001) manually as
e11f878a3569f9e316c478d2e5595e0b000d0720 (presubmit successful).