Lower a few basic MIPS binops for i{8,16,32,64}.

Lower a few basic MIPS binops for i{8,16,32,64}.

This is basically the same patch as for ARM issue 1127003003

https://codereview.chromium.org/1127003003

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

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

[rkotlerimgtec, 2015-11-03 03:45:39]

Description was changed from

==========
Lower a few basic MIPS binops for i{8,16,32,64}.

This is basically the same patch as for ARM issue 1127003003

https://codereview.chromium.org/1127003003

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

to

==========
Lower a few basic MIPS binops for i{8,16,32,64}.

This is basically the same patch as for ARM issue 1127003003

https://codereview.chromium.org/1127003003

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

[rkotlerimgtec, 2015-11-03 03:49:13]

I omitted the 64 bit mul test because I need to implement some additional
mechanism to handle lo/hi arithmetic accumulator. In addition I discovered a bug
unrelated to this patch regarding store/load from the stack and will fix that in
a separate patch and that made it impossible to pass the Om1 test case. Move
must be changed to lw/sw when referencing the stack.

The patch follows closely the original ARM patch and does not attempt to
implement functionality not in that patch.

[rkotlerimgtec, 2015-11-04 00:09:55]

On 2015/11/03 03:49:13, rkotlerimgtec wrote:
> I omitted the 64 bit mul test because I need to implement some additional
> mechanism to handle lo/hi arithmetic accumulator. In addition I discovered a
bug
> unrelated to this patch regarding store/load from the stack and will fix that
in
> a separate patch and that made it impossible to pass the Om1 test case. Move
> must be changed to lw/sw when referencing the stack.
> 
> The patch follows closely the original ARM patch and does not attempt to
> implement functionality not in that patch.

I have the patch now for this lw/sw issue; changing move to lw/sw for stack
variables.
It's not related to this patch but with that fixed, the -Om test will pass for
arith.ll

[Jim Stichnoth, 2015-11-04 14:47:24]

stichnot@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@google.com

[Jim Stichnoth, 2015-11-04 14:47:24]

Reed - please be sure to include John and Karl as reviewers on the MIPS CLs, as
they are deep in the Subzero ARM target.

https://codereview.chromium.org/1414383004/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1414383004/diff/1/src/IceInstMIPS32.h#newcode271
src/IceInstMIPS32.h:271: bool SetFlags;
This isn't initialized anywhere.  I think it needs to be an argument to the ctor
and the create() method, like in its ARM counterpart.

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

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:529: } else if (isVectorType(Dest->getType())) {
Don't use "else if" when the "if" branch ends with a return.  It's easier to
reason about the code that follows.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:532: Variable *T = makeReg(Dest->getType());
Now is the part where I wish I had learned more about MIPS and ARM instruction
sets.

If I understand correctly, ARM generally allows one source operand of an
arithmetic instruction to be a memory operand, whereas MIPS requires both source
operands to be registers (ignoring immediate operands).

If this is true, then you should do a couple of things.  First, change
InstMIPS32ThreeAddrGPR so that the Src1 operand in its ctor and create() methods
is a Variable*, rather than Operand*.  Second, treat Src1R below similar to
Src0R, e.g.:

  Variable *Src1R = legalizeToReg(Src1);

Note that I'm assuming that the arithmetic instruction variants where the second
source operand is an immediate, are represented as separate instruction types
like in ARM.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:537: Context.insert(InstFakeUse::create(Func,
Src0R));
I think the FakeUse and FakeDef instructions should go *after* the switch
statement.

For a fully implemented instruction, like Add, we do the right stuff and return,
and no fake use/def is required.

So after the switch statement, we can set up the fake uses/defs which are now
necessary.

First, add the FakeUses on Src0R and Src1R.  This is necessary because the
legalizeToReg() calls may generate assigments to new infinite-weight
temporaries, and without corresponding uses of those temporaries within the same
basic block, the Om1 register allocator will complain.  (This complaint is not
strictly necessary for correctness, but it is a possible signal of incorrect
lowering, so we keep it.)

Second, add the FakeDef on Dest, and then signal the UnimplementedError.

Then you can fill in the implementations of the other arithmetic instruction
kinds as you go along, and eventually remove the FakeDef, FakeUse, and
UnimplementedError.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:567: Variable *Src1R_ = legalizeToReg(Src1R);
If you follow the comment above and declare Src1R as Variable*, then Src1R_ is
unnecessary and you can just use Src1R.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:94: // The following are helpers that insert
lowered ARM32 instructions with
MIPS

https://codereview.chromium.org/1414383004/diff/1/tests_lit/llvm2ice_tests/ar...
File tests_lit/llvm2ice_tests/arith.ll (right):

https://codereview.chromium.org/1414383004/diff/1/tests_lit/llvm2ice_tests/ar...
tests_lit/llvm2ice_tests/arith.ll:27: ; RUN:   --command %p2i --filetype=asm
--assemble --disassemble --target mips32 \
80-col

[rkotlerimgtec, 2015-11-05 01:28:44]

In addition to changes requested by stichnot, I also fixed the issue with the
mov instruction so now the -Om1 test can work.

https://codereview.chromium.org/1414383004/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1414383004/diff/1/src/IceInstMIPS32.h#newcode271
src/IceInstMIPS32.h:271: bool SetFlags;
On 2015/11/04 14:47:24, stichnot wrote:
> This isn't initialized anywhere.  I think it needs to be an argument to the
ctor
> and the create() method, like in its ARM counterpart.

This variable is not needed. Was copied from ARM part and is not used, except in
one other place in a dump routine which needs to be deleted because it is not
needed.

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

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:529: } else if (isVectorType(Dest->getType())) {
On 2015/11/04 14:47:24, stichnot wrote:
> Don't use "else if" when the "if" branch ends with a return.  It's easier to
> reason about the code that follows.

Done.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:532: Variable *T = makeReg(Dest->getType());
On 2015/11/04 14:47:24, stichnot wrote:
> Now is the part where I wish I had learned more about MIPS and ARM instruction
> sets.
> 
> If I understand correctly, ARM generally allows one source operand of an
> arithmetic instruction to be a memory operand, whereas MIPS requires both
source
> operands to be registers (ignoring immediate operands).
> 
> If this is true, then you should do a couple of things.  First, change
> InstMIPS32ThreeAddrGPR so that the Src1 operand in its ctor and create()
methods
> is a Variable*, rather than Operand*.  Second, treat Src1R below similar to
> Src0R, e.g.:
> 
>   Variable *Src1R = legalizeToReg(Src1);
> 
> Note that I'm assuming that the arithmetic instruction variants where the
second
> source operand is an immediate, are represented as separate instruction types
> like in ARM.

Done.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:537: Context.insert(InstFakeUse::create(Func,
Src0R));
On 2015/11/04 14:47:24, stichnot wrote:
> I think the FakeUse and FakeDef instructions should go *after* the switch
> statement.
> 
> For a fully implemented instruction, like Add, we do the right stuff and
return,
> and no fake use/def is required.
> 
> So after the switch statement, we can set up the fake uses/defs which are now
> necessary.
> 
> First, add the FakeUses on Src0R and Src1R.  This is necessary because the
> legalizeToReg() calls may generate assigments to new infinite-weight
> temporaries, and without corresponding uses of those temporaries within the
same
> basic block, the Om1 register allocator will complain.  (This complaint is not
> strictly necessary for correctness, but it is a possible signal of incorrect
> lowering, so we keep it.)
> 
> Second, add the FakeDef on Dest, and then signal the UnimplementedError.
> 
> Then you can fill in the implementations of the other arithmetic instruction
> kinds as you go along, and eventually remove the FakeDef, FakeUse, and
> UnimplementedError.

Done.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:567: Variable *Src1R_ = legalizeToReg(Src1R);
On 2015/11/04 14:47:24, stichnot wrote:
> If you follow the comment above and declare Src1R as Variable*, then Src1R_ is
> unnecessary and you can just use Src1R.

Done.

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/1414383004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:94: // The following are helpers that insert
lowered ARM32 instructions with
On 2015/11/04 14:47:24, stichnot wrote:
> MIPS

Done.

https://codereview.chromium.org/1414383004/diff/1/tests_lit/llvm2ice_tests/ar...
File tests_lit/llvm2ice_tests/arith.ll (right):

https://codereview.chromium.org/1414383004/diff/1/tests_lit/llvm2ice_tests/ar...
tests_lit/llvm2ice_tests/arith.ll:27: ; RUN:   --command %p2i --filetype=asm
--assemble --disassemble --target mips32 \
On 2015/11/04 14:47:24, stichnot wrote:
> 80-col

Done.

[Jim Stichnoth, 2015-11-05 05:21:28]

https://codereview.chromium.org/1414383004/diff/20001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1414383004/diff/20001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:250: auto *Dest = getDest();
Please use Variable* here and Operand* in the next statement, instead of auto.

It's OK to use auto in the dyn_cast statement because otherwise the type is
being repeated.

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

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:530:
UnimplementedError(Func->getContext()->getFlags());
You may want to add this for the future:

  Context.insert(InstFakeDef::create(Func, Dest));

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:531: } else { // Dest->getType() is non-i64
scalar
Here, I think you can do:

  if (isVectorType(Dest->getType())) {
    UnimplementedError(Func->getContext()->getFlags());
    return;
  }

and then the rest of the function doesn't need to be inside an "else" branch.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:534: Operand *Src1R_ = legalize(Src1,
Legal_Reg);
I think you can just do:

  Variable *Src1R = legalizeToReg(Src1);

then remove everything up to the start of the switch statement.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:568: // Variable *Src1R_ = legalizeToReg(Src1R);
remove comment

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:601: if (auto *S1 =
llvm::dyn_cast<Variable>(Src1R)) {
After changing Src1R to be Variable* above, you should be able to
unconditionally insert the FakeUse.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1021: return From;
This is causing a problem for function @ShlReloc in arith.ll.  If we ask to
legalize a ConstantRelocatable to a register, we should probably be doing the
lui/ori trick on the upper and lower parts of the ConstantRelocatable.

Or to make quicker progress, fake it like this:

    Variable *Reg = makeReg(Ty, RegNum);
    Context.insert(InstFakeDef::create(Func, Reg));
    return Reg;

[rkotlerimgtec, 2015-11-05 22:12:28]

https://codereview.chromium.org/1414383004/diff/20001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1414383004/diff/20001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:250: auto *Dest = getDest();
On 2015/11/05 05:21:28, stichnot wrote:
> Please use Variable* here and Operand* in the next statement, instead of auto.
> 
> It's OK to use auto in the dyn_cast statement because otherwise the type is
> being repeated.

Done.

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

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:530:
UnimplementedError(Func->getContext()->getFlags());
On 2015/11/05 05:21:28, stichnot wrote:
> You may want to add this for the future:
> 
>   Context.insert(InstFakeDef::create(Func, Dest));

Done.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:531: } else { // Dest->getType() is non-i64
scalar
On 2015/11/05 05:21:28, stichnot wrote:
> Here, I think you can do:
> 
>   if (isVectorType(Dest->getType())) {
>     UnimplementedError(Func->getContext()->getFlags());
>     return;
>   }
> 
> and then the rest of the function doesn't need to be inside an "else" branch.

Done.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:534: Operand *Src1R_ = legalize(Src1,
Legal_Reg);
On 2015/11/05 05:21:28, stichnot wrote:
> I think you can just do:
> 
>   Variable *Src1R = legalizeToReg(Src1);
> 
> then remove everything up to the start of the switch statement.

Done.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:568: // Variable *Src1R_ = legalizeToReg(Src1R);
On 2015/11/05 05:21:28, stichnot wrote:
> remove comment

Done.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:601: if (auto *S1 =
llvm::dyn_cast<Variable>(Src1R)) {
On 2015/11/05 05:21:28, stichnot wrote:
> After changing Src1R to be Variable* above, you should be able to
> unconditionally insert the FakeUse.

Done.

https://codereview.chromium.org/1414383004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1021: return From;
On 2015/11/05 05:21:28, stichnot wrote:
> This is causing a problem for function @ShlReloc in arith.ll.  If we ask to
> legalize a ConstantRelocatable to a register, we should probably be doing the
> lui/ori trick on the upper and lower parts of the ConstantRelocatable.
> 
> Or to make quicker progress, fake it like this:
> 
>     Variable *Reg = makeReg(Ty, RegNum);
>     Context.insert(InstFakeDef::create(Func, Reg));
>     return Reg;

Done.

[Jim Stichnoth, 2015-11-06 00:45:15]

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:246: void InstMIPS32Mov::emitSingleDestSingleSource(const
Cfg *Func) const {
For these textual emit/dump routines, add "if (!BuildDefs::dump()) return;" at
the beginning, to minimize binary size for a MINIMAL build.

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:248: // assert(Inst->getSrcSize() == 1);
Remove these comments.

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

https://codereview.chromium.org/1414383004/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1011: Variable *Reg = makeReg(Ty, RegNum);
Also add a TODO for a proper implementation.

[rkotlerimgtec, 2015-11-06 00:58:18]

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:246: void InstMIPS32Mov::emitSingleDestSingleSource(const
Cfg *Func) const {
On 2015/11/06 00:45:15, stichnot wrote:
> For these textual emit/dump routines, add "if (!BuildDefs::dump()) return;" at
> the beginning, to minimize binary size for a MINIMAL build.

Done.

https://codereview.chromium.org/1414383004/diff/40001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:248: // assert(Inst->getSrcSize() == 1);
On 2015/11/06 00:45:15, stichnot wrote:
> Remove these comments.

Done.

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

https://codereview.chromium.org/1414383004/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1011: Variable *Reg = makeReg(Ty, RegNum);
On 2015/11/06 00:45:15, stichnot wrote:
> Also add a TODO for a proper implementation.

Done.

[Jim Stichnoth, 2015-11-06 01:07:28]

Committed patchset #4 (id:60001) manually as
37af5b0f20ebe22be29de46158dabed97db08cff (presubmit successful).