[Subzero][MIPS32] Implement sext, zext and trunc

[Subzero][MIPS32] Implement sext, zext and trunc

This patch adds support for sext, zext, trunc operations on i8, i16, i32 source operand types. Support for i1 source operand type will follow.

R=stichnot@chromium.org

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

[sagar.thakur, 2016-05-04 18:28:57]

Description was changed from

==========
[Subzero][MIPS32] Implement sext, zext and trunc
==========

to

==========
[Subzero][MIPS32] Implement sext, zext and trunc

This patch adds support for sext, zext, trunc operations on i8, i16, i32 source
operand types. Support for i1 source operand type will follow.
==========

[sagar.thakur, 2016-05-04 18:28:57]

sagar.thakur@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
stichnot@chromium.org

[Jim Stichnoth, 2016-05-05 15:32:38]

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:910: Operand *Src0 =
legalizeUndef(Instr->getSrc(0));
Add this:
  const Type Src0Ty = Src0->getType();
and use it below in place of all the Src0->getType() calls.

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:928: _sll(T_Lo, Src0R, 24);
I strongly prefer to avoid magic constants like 24 and 16.  Instead, something
like this:
  constexpr ShiftAmount = 32 - (CHAR_BIT * typeWidthInBytes(Src0Ty));
Sadly, that trades one magic constant for another, but I guess 32 could be a
static constexpr field in the TargetMIPS32 class.

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:960: _andi(DestLo, Src0R, 0xff);
Shouldn't the mask be 0xffff for i16 and 0xff for i8?

Also, similar to above, these masks could be constructed based on the type width
instead of "magic constants".

[sagar.thakur, 2016-05-06 13:04:06]

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:910: Operand *Src0 =
legalizeUndef(Instr->getSrc(0));
On 2016/05/05 15:32:37, stichnot wrote:
> Add this:
>   const Type Src0Ty = Src0->getType();
> and use it below in place of all the Src0->getType() calls.

Done.

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:928: _sll(T_Lo, Src0R, 24);
On 2016/05/05 15:32:37, stichnot wrote:
> I strongly prefer to avoid magic constants like 24 and 16.  Instead, something
> like this:
>   constexpr ShiftAmount = 32 - (CHAR_BIT * typeWidthInBytes(Src0Ty));
> Sadly, that trades one magic constant for another, but I guess 32 could be a
> static constexpr field in the TargetMIPS32 class.

Done.

https://codereview.chromium.org/1948093002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:960: _andi(DestLo, Src0R, 0xff);
On 2016/05/05 15:32:37, stichnot wrote:
> Shouldn't the mask be 0xffff for i16 and 0xff for i8?
> 
> Also, similar to above, these masks could be constructed based on the type
width
> instead of "magic constants".

Done.

[Jim Stichnoth, 2016-05-06 19:55:10]

https://codereview.chromium.org/1948093002/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1948093002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:465: InstMIPS32Imm16(Cfg *Func, Variable *Dest, Operand
*Source,
Why is this new form needed?

It seems to me that if the specific MIPS instruction requires an Imm16 operand,
you might as well keep it simple and just use the uint32_t form?

It's really up to the TargetLowering implementation (e.g. MIPS32) to decide how
to handle these things, so I just wanted to understand the assumptions/direction
here.

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:920:
llvm::cast<ConstantInteger32>(Ctx->getConstantInt32(31));
Maybe INT32_BITS-1 ?

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:940: } else if (Src0Ty == IceType_i8) {
Combine this and the next clause:

  else if (Src0Ty == IceType_i8 || Src0Ty == IceType_i16) {

Here and below.

[sagar.thakur, 2016-05-09 11:03:40]

https://codereview.chromium.org/1948093002/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1948093002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:465: InstMIPS32Imm16(Cfg *Func, Variable *Dest, Operand
*Source,
On 2016/05/06 19:55:09, stichnot wrote:
> Why is this new form needed?
> 
> It seems to me that if the specific MIPS instruction requires an Imm16
operand,
> you might as well keep it simple and just use the uint32_t form?
> 
> It's really up to the TargetLowering implementation (e.g. MIPS32) to decide
how
> to handle these things, so I just wanted to understand the
assumptions/direction
> here.

I was following arm and x86 implementation of TargetLowering. Yes, this is
actually not needed here. I'll just use the uint32_t form.

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:920:
llvm::cast<ConstantInteger32>(Ctx->getConstantInt32(31));
On 2016/05/06 19:55:10, stichnot wrote:
> Maybe INT32_BITS-1 ?

Done.

https://codereview.chromium.org/1948093002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:940: } else if (Src0Ty == IceType_i8) {
On 2016/05/06 19:55:09, stichnot wrote:
> Combine this and the next clause:
> 
>   else if (Src0Ty == IceType_i8 || Src0Ty == IceType_i16) {
> 
> Here and below.

Done. Sorry I didn't notice that.

[Jim Stichnoth, 2016-05-09 18:41:27]

lgtm

https://codereview.chromium.org/1948093002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1948093002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:952: switch (Src0Ty) {
It would probably be cleaner if all of the similar tests would all use switch
statements, or all use if/then/else, but that can be done later after all the
dust settles.

[Jim Stichnoth, 2016-05-09 18:58:04]

Description was changed from

==========
[Subzero][MIPS32] Implement sext, zext and trunc

This patch adds support for sext, zext, trunc operations on i8, i16, i32 source
operand types. Support for i1 source operand type will follow.
==========

to

==========
[Subzero][MIPS32] Implement sext, zext and trunc

This patch adds support for sext, zext, trunc operations on i8, i16, i32 source
operand types. Support for i1 source operand type will follow.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-05-09 18:58:05]

Committed patchset #3 (id:40001) manually as
38dcb59d93d2db52cb29bcd64c8029ccd7d5072e (presubmit successful).