MIPS: Add turbofan support for mips32.

MIPS: Add turbofan support for mips32.

BUG=
R=bmeurer@chromium.org, dusan.milosavljevic@imgtec.com

Committed: https://code.google.com/p/v8/source/detail?r=24397

[paul.l..., 2014-09-24 14:26:06]

paul.lind@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com, bmeurer@chromium.org,
danno@chromium.org, dusan.milosavljevic@imgtec.com, titzer@chromium.org

[paul.l..., 2014-09-24 14:26:07]

PTAL. (I'll add the new float32 backend to this later today, possibly as a 2nd
CL).

[dusmil.imgtec, 2014-09-24 15:03:52]

Mips part lgtm.

[titzer, 2014-09-24 15:23:32]

Hi Paul,

Awesome that you've done all this work already! This is much faster than I had
even dreamed of. And yes we are ready for it :D

Here's an initial round of comments from me.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:108: //    messy macro? Consider
expanding this in place for sll, srl, sra ops.
I agree that you should kill this macro here.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:170: i.InputOperand(1), kCompareReg,
kScratchReg);
The macro assembler methods here generate a lot of instructions. It may not be
an overall win to generate an add with overflow on MIPS, but instead to generate
an explicit tag check with a branch and a regular add.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:361: FlagsCondition condition) {
The handling of branches here is tough to follow. It may be that the flags
continuation which works OK on other architectures doesn't work so great here.
In particular it seems that materializing the extra bit for AddWithOverflow,
only to feed it into a branch at the end is a bit indirect.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
File src/compiler/mips/instruction-codes-mips.h (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-codes-mips.h:34: V(MipsFloat64Cmp)               
\
Consider naming these opcodes as close as possible to the underlying
Assembler/MacroAssembler method that they end up calling. We didn't do that on
arm but I think we want to clean that up in the future.

https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips...
src/mips/code-stubs-mips.cc:1103: // a1: pointer to builtin function
Changes here and other changes outside of TurboFan already look fine (i.e. ready
to land).

Would you consider splitting those out into a separate CL and landing them
first?

[paul.l..., 2014-09-25 03:53:32]

Hi Ben - Thanks for getting to the review so quickly, for your kind words, and
for the good feedback you gave. I've factored out the changes to the base mips
port files as you requested, and those are already landed. I've rebased this
patch, and have implemented the sqrt and float32 stuff that landed today. I've
run into bugs on my float32 implementation, and I'll have to resume with that in
the morning.

You had several good suggestions that I'll implement immediately, and some
questions about overflow detection and branch handling that get a little deeper.
I'd agree that these need improvement, but there are some constraints and
issues. I'll give you my feedback tomorrow, and then we can decide the proper
direction to move forward on these.

thanks!

[Benedikt Meurer, 2014-09-25 05:22:57]

Hey Paul, I'm adding the missing bits for Float32Constant currently. Should be
in during the day. I'd suggest to wait for that and rebase on top of it.

[Benedikt Meurer, 2014-09-25 05:37:11]

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:361: FlagsCondition condition) {
Hm, we may need to refactor the AssembleArchBranch and AssembleArchBoolean logic
a bit to make it easier for MIPS. I'll think about that.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
File src/compiler/mips/instruction-codes-mips.h (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-codes-mips.h:34: V(MipsFloat64Cmp)               
\
On 2014/09/24 at 15:23:32, titzer wrote:
> Consider naming these opcodes as close as possible to the underlying
Assembler/MacroAssembler method that they end up calling. We didn't do that on
arm but I think we want to clean that up in the future.

Yep, it's on my TODO list for ARM.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
File src/compiler/mips/instruction-selector-mips-unittest.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:20: 
You should add std::ostream& operator<<(std::ostream&, const MachInst<T>&) for
this, otherwise test failures will be hard to interpret.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:37: static const FPCmp
kFPCmpInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:66: static const
MachInst2 kLogicalInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:80: static const
MachInst2 kShiftInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:96: static const
MachInst2 kMulDivInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:111: static const
MachInst2 kModInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:123: static const
MachInst2 kFPArithInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:135: static const
MachInst2 kAddSubInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:149: static const
MachInst1 kAddSubOneInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:161: static const IntCmp
kCmpInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:194: static const
Conversion kConversionInstructions[] = {
Nit: no need to use static, it's in an anonymous namespace already.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:512: 
You should add

std::ostream& operator<<(std::ostream& os, const MemoryAccessImm& acc) {
  OStringStream ost;
  ost << acc.type;
  return os << ost.c_str();
}

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:522: 
Stream operator, see above.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:529: static const
MemoryAccessImm kMemoryAccessesImm[] = {
Nit: static, see above.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:581: static const
MemoryAccessImm1 kMemoryAccessImmMoreThan16bit[] = {
Nit: static, see above.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:795: 
Nit: extra new line :-)

[Michael Starzinger, 2014-09-25 09:05:11]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2014-09-25 09:05:12]

The changes outside the "compiler" directory look good, I'll leave the review of
the backend to Ben and Benedikt.

[paul.l..., 2014-09-25 17:18:40]

Hey guys, thanks for all your feedback! Most things have been addressed, though
a couple are still up for further discussion.

This is NOT ready for landing yet, still need to rebase to today and add
Benedikt's float32 constant change. And I have one small bug still to chase.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:108: //    messy macro? Consider
expanding this in place for sll, srl, sra ops.
On 2014/09/24 15:23:31, titzer wrote:
> I agree that you should kill this macro here.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:170: i.InputOperand(1), kCompareReg,
kScratchReg);
On 2014/09/24 15:23:31, titzer wrote:
> The macro assembler methods here generate a lot of instructions. It may not be
> an overall win to generate an add with overflow on MIPS, but instead to
generate
> an explicit tag check with a branch and a regular add.

Yes, Overflow checking is an issue for MIPS. We have two variations of add/sub,
one generates TRAP on overflow[1], the other silently overflows. We use the
silent version, but must generate extra instructions to check. I don't
understand your suggestion "generate an explicit tag check with a branch and a
regular add", can you elaborate?

The new MIPS r6 instruction set gives us a new/faster way to detect overflows,
we will implement that soon, but still need the old way for older cores.

[1] I prototyped with the TRAP version a year ago, and it made a nice speed
improvement on some benchmarks, but I thought it best to avoid installing signal
handler, it could be fragile, and maybe problematic with sandboxing.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:361: FlagsCondition condition) {
Complications arise from MIPS not having condition codes like other arch's. The
compare and branch are merged into a single instruction, which means that the
branch instruction needs the registers provided to the compare to still be
valid. The current FlagsContinuation mechanism does guarantee this. As you can
see we mostly don't emit instructions for the compare itself, but we do preserve
the type of comparison as a psuedo-op to guide the branch generation.

> The handling of branches here is tough to follow.

Yes, the code is a bit ugly :(  It "looked better" here with single-line cases: 
https://github.com/paul99/v8m-rb/blob/b8e8df9/src/compiler/mips/code-generato...
but clang-format won't take that form. I'm thinking of splitting into 4 smaller
routines, each specialized for one arch_opcode type. I'd be glad to hear your
ideas. 

Another issue is the exposed branch delay slots here, which make things faster
at the expense of readability.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
File src/compiler/mips/instruction-codes-mips.h (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-codes-mips.h:34: V(MipsFloat64Cmp)               
\
On 2014/09/25 05:37:10, Benedikt Meurer wrote:
> On 2014/09/24 at 15:23:32, titzer wrote:
> > Consider naming these opcodes as close as possible to the underlying
> Assembler/MacroAssembler method that they end up calling. We didn't do that on
> arm but I think we want to clean that up in the future.
> 
> Yep, it's on my TODO list for ARM.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
File src/compiler/mips/instruction-selector-mips-unittest.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:20: 
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> You should add std::ostream& operator<<(std::ostream&, const MachInst<T>&) for
> this, otherwise test failures will be hard to interpret.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:37: static const FPCmp
kFPCmpInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:66: static const
MachInst2 kLogicalInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:80: static const
MachInst2 kShiftInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:96: static const
MachInst2 kMulDivInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:111: static const
MachInst2 kModInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:123: static const
MachInst2 kFPArithInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:135: static const
MachInst2 kAddSubInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:149: static const
MachInst1 kAddSubOneInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:161: static const IntCmp
kCmpInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:194: static const
Conversion kConversionInstructions[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: no need to use static, it's in an anonymous namespace already.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:512: 
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> You should add
> 
> std::ostream& operator<<(std::ostream& os, const MemoryAccessImm& acc) {
>   OStringStream ost;
>   ost << acc.type;
>   return os << ost.c_str();
> }

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:522: 
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Stream operator, see above.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:529: static const
MemoryAccessImm kMemoryAccessesImm[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: static, see above.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:581: static const
MemoryAccessImm1 kMemoryAccessImmMoreThan16bit[] = {
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: static, see above.

Done.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/instru...
src/compiler/mips/instruction-selector-mips-unittest.cc:795: 
On 2014/09/25 05:37:11, Benedikt Meurer wrote:
> Nit: extra new line :-)

Done.

https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/mips/code-stubs-mips...
src/mips/code-stubs-mips.cc:1103: // a1: pointer to builtin function
On 2014/09/24 15:23:32, titzer wrote:
> Changes here and other changes outside of TurboFan already look fine (i.e.
ready
> to land).
> 
> Would you consider splitting those out into a separate CL and landing them
> first?

Done.

[paul.l..., 2014-09-26 00:15:30]

PTAL, it's rebased, comments addressed, and passes all the tests.

[paul.l..., 2014-09-26 00:30:18]

I should add that this CL does not yet attempt to fix the somewhat ugly but
functional AssembleArchBranch()/Boolean(). I do have one improvement in mind
there, as discussed in comment #10 above, and I'm certainly open to suggestions.
IMO that could be a follow-on CL.

[Benedikt Meurer, 2014-09-26 06:37:01]

On 2014/09/26 at 00:30:18, paul.lind wrote:
> I should add that this CL does not yet attempt to fix the somewhat ugly but
functional AssembleArchBranch()/Boolean(). I do have one improvement in mind
there, as discussed in comment #10 above, and I'm certainly open to suggestions.
IMO that could be a follow-on CL.

Yep, sounds reasonable.

[titzer, 2014-09-26 10:36:21]

Looks good from my side.

Benedikt?

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:170: i.InputOperand(1), kCompareReg,
kScratchReg);
On 2014/09/25 17:18:39, paul.l... wrote:
> On 2014/09/24 15:23:31, titzer wrote:
> > The macro assembler methods here generate a lot of instructions. It may not
be
> > an overall win to generate an add with overflow on MIPS, but instead to
> generate
> > an explicit tag check with a branch and a regular add.
> 
> Yes, Overflow checking is an issue for MIPS. We have two variations of
add/sub,
> one generates TRAP on overflow[1], the other silently overflows. We use the
> silent version, but must generate extra instructions to check. I don't
> understand your suggestion "generate an explicit tag check with a branch and a
> regular add", can you elaborate?
> 

I meant in the front end, when we lower a v8 representation change (i.e. convert
a tagged number to a machine int32 or vice versa). If AddWithOverflow isn't very
efficient on a particular backend, we can generate a different set of machine
operators. That's something where the backend should be able to configure the
frontend to avoid expensive operations.

> The new MIPS r6 instruction set gives us a new/faster way to detect overflows,
> we will implement that soon, but still need the old way for older cores.
> 

Ok, that is more reason to have the backend configure the frontend to avoid
certain operations that are complex/slow on specific generations of an
architecture.

> [1] I prototyped with the TRAP version a year ago, and it made a nice speed
> improvement on some benchmarks, but I thought it best to avoid installing
signal
> handler, it could be fragile, and maybe problematic with sandboxing. 
> 

Agree. And it could also be very slow for certain programs.

https://codereview.chromium.org/601723002/diff/20001/src/compiler/mips/code-g...
src/compiler/mips/code-generator-mips.cc:361: FlagsCondition condition) {
On 2014/09/25 17:18:39, paul.l... wrote:
> Complications arise from MIPS not having condition codes like other arch's.
The
> compare and branch are merged into a single instruction, which means that the
> branch instruction needs the registers provided to the compare to still be
> valid. The current FlagsContinuation mechanism does guarantee this. As you can
> see we mostly don't emit instructions for the compare itself, but we do
preserve
> the type of comparison as a psuedo-op to guide the branch generation.
> 
> > The handling of branches here is tough to follow.
> 
> Yes, the code is a bit ugly :(  It "looked better" here with single-line
cases: 
>
https://github.com/paul99/v8m-rb/blob/b8e8df9/src/compiler/mips/code-generato...
> but clang-format won't take that form. I'm thinking of splitting into 4
smaller
> routines, each specialized for one arch_opcode type. I'd be glad to hear your
> ideas. 
> 
> Another issue is the exposed branch delay slots here, which make things faster
> at the expense of readability.

Ok, in that case maybe using a FlagsContinuation on MIPS is more trouble than
its worth, and the instruction selector could just emit specific kinds of
compare instructions and not try to combine the flags usage with arithmetic
(which is our goal on x86).

But that's something we can iterate on; I'm ok with landing this as is.

[paul.l..., 2014-09-29 13:39:51]

On 2014/09/26 10:36:21, titzer wrote:
> Looks good from my side.
> 
> Benedikt?

Benedikt, any other changes you'd like to see in this initial version?

[paul.l..., 2014-10-01 13:56:03]

Hi Ben, Benedikt - Can this get a approved now, or is there something else you
would like fixed in this initial version? (I do have some minor rebasing for
today's changes)

[paul.l..., 2014-10-01 23:14:32]

Rebased to today. PTAL.

[Benedikt Meurer, 2014-10-02 08:15:46]

LGTM

[paul.l..., 2014-10-02 15:48:59]

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