Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

This patch extends InstMIPS32Mov instruction to support different datatypes, and emit proper low level instruction depending on operands properties and data types.

R=stichnot@chromium.org

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

[obucinac, 2016-07-05 17:36:04]

Description was changed from

==========
Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

This patch extends InstMIPS32Mov instruction to support different
datatypes. This way, we can at least in some way match problems
inherited from following other implementations too closely.
==========

to

==========
Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

This patch extends InstMIPS32Mov instruction to support different datatypes, and
emit proper low level instruction depending on operands properties and data
types.
==========

[obucinac, 2016-07-05 17:36:04]

obucinac@gmail.com changed reviewers:
+ kschimpf@google.com, mohit.bhakkad@imgtec.com, sagar.thakur@imgtec.com,
stichnot@chromium.org

[obucinac, 2016-07-05 18:17:28]

The patch is also tested with slightly changed patch
https://codereview.chromium.org/2027773002/ (Subzero, MIPS32: Handling floating
point instructions fadd, fsub, fmul, fdiv), and it produces proper result, so
this may be the prerequisite we were waiting for it.

[Jim Stichnoth, 2016-07-06 11:18:58]

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:558: const char *ActualOpcode = NULL;
nullptr

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:559: bool DestIsReg = Dest->hasReg();
const bool
for these 4 variables

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:562: bool SrcIsMem = !(SrcV && SrcV->hasReg());
Just checking - what is Src is a Constant, e.g. "42"?
Is that guaranteed to be legalized away during lowering?
Should that be asserted here?

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:576: Str << "\t";
Str << "\t" "add" "\t";
to use compile-time string concatenation when possible.

In case it's not clear, I prefer not to use "\tadd\t" because then you can find
these strings more easily using
  git grep -w add
  git grep '"add"'

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:599: Str << "\t";
Str << "\t" << ActualOpcode << "\t";
here and below

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:612: UnimplementedError(getFlags());
add "break" after this?

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:687: UnimplementedError(getFlags());
Should this be a hard error?  E.g. in x86 it would be, because mem-to-mem moves
are generally not possible and so the lowering should have legalized it to
something valid.

[obucinac, 2016-07-06 13:40:53]

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:558: const char *ActualOpcode = NULL;
On 2016/07/06 11:18:57, stichnot wrote:
> nullptr

Done.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:559: bool DestIsReg = Dest->hasReg();
On 2016/07/06 11:18:57, stichnot wrote:
> const bool
> for these 4 variables

Done.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:562: bool SrcIsMem = !(SrcV && SrcV->hasReg());
On 2016/07/06 11:18:57, stichnot wrote:
> Just checking - what is Src is a Constant, e.g. "42"?
> Is that guaranteed to be legalized away during lowering?
> Should that be asserted here?

All constants are lowered to OperandMIPS32Mem. We may add assert check anyway.
Mohit covers this.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:576: Str << "\t";
On 2016/07/06 11:18:58, stichnot wrote:
> Str << "\t" "add" "\t";
> to use compile-time string concatenation when possible.
> 
> In case it's not clear, I prefer not to use "\tadd\t" because then you can
find
> these strings more easily using
>   git grep -w add
>   git grep '"add"'

Done.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:599: Str << "\t";
On 2016/07/06 11:18:57, stichnot wrote:
> Str << "\t" << ActualOpcode << "\t";
> here and below

Done.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:612: UnimplementedError(getFlags());
On 2016/07/06 11:18:58, stichnot wrote:
> add "break" after this?

Done.

https://codereview.chromium.org/2122043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:687: UnimplementedError(getFlags());
On 2016/07/06 11:18:57, stichnot wrote:
> Should this be a hard error?  E.g. in x86 it would be, because mem-to-mem
moves
> are generally not possible and so the lowering should have legalized it to
> something valid.

This case will be covered in lowerAssign, by copying through temporary register.
Currently, no test case emits mov with two stack operands (although in previous
version there is a case for it).

[obucinac, 2016-07-06 13:41:58]

https://codereview.chromium.org/2122043002/diff/20001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (left):

https://codereview.chromium.org/2122043002/diff/20001/src/IceInstMIPS32.cpp#o...
src/IceInstMIPS32.cpp:566: "\t";
This is done by format tool

[Jim Stichnoth, 2016-07-06 22:57:50]

lgtm

[Jim Stichnoth, 2016-07-06 22:58:11]

Description was changed from

==========
Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

This patch extends InstMIPS32Mov instruction to support different datatypes, and
emit proper low level instruction depending on operands properties and data
types.
==========

to

==========
Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

This patch extends InstMIPS32Mov instruction to support different datatypes, and
emit proper low level instruction depending on operands properties and data
types.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-07-06 22:58:13]

Committed patchset #2 (id:20001) manually as
36847bdd4ac98503242b74444011e6bb6cd6bdd6 (presubmit successful).

[obucinac, 2016-07-07 13:35:53]

On 2016/07/06 22:58:13, stichnot wrote:
> Committed patchset #2 (id:20001) manually as
> 36847bdd4ac98503242b74444011e6bb6cd6bdd6 (presubmit successful).

Patch is changed as suggested by Simon Dardis.

The background for this is that 'or' is better than 'add' for 64bit cores
running O32 code as 'add' is required to sign-extend its result whereas 'or'
doesn't.

[Jim Stichnoth, 2016-07-07 16:24:00]

On 2016/07/07 13:35:53, obucinac wrote:
> On 2016/07/06 22:58:13, stichnot wrote:
> > Committed patchset #2 (id:20001) manually as
> > 36847bdd4ac98503242b74444011e6bb6cd6bdd6 (presubmit successful).
> 
> Patch is changed as suggested by Simon Dardis.
> 
> The background for this is that 'or' is better than 'add' for 64bit cores
> running O32 code as 'add' is required to sign-extend its result whereas 'or'
> doesn't.

Sorry, since the earlier patch is already landed, this new change will need to
be done as a new CL or add part of another CL.