MIPS: Optimise Math.floor(x/y) to use integer division for MIPS.

MIPS: Optimise Math.floor(x/y) to use integer division for MIPS.

Use div instruction if some divisors do not have magic number.

Based on commit r11427 (318a9598).

This commit also ports commit r15161 (554d45c1).

BUG=

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

[palfia, 2013-06-14 16:04:45]

Hi Jakob,

This change also contains the port of r15161.

PTAL.

[Jakob Kummerow, 2013-06-14 16:33:33]

Hydrogen and mjsunit changes LGTM. I have not reviewed the src/mips/* changes in
detail.

You'll need to rebase because I just fixed something in the Hydrogen method, but
the changes should resolve gracefully.

[Paul Lind, 2013-06-14 16:34:58]

The MIPS part LGTM, if you address the 2 comments.

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips.cc
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips...
src/mips/lithium-codegen-mips.cc:1187: 
Is there any specific reason you removed this assertion here:
ASSERT(LChunkBuilder::HasMagicNumberForDivisor(divisor));

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips...
src/mips/lithium-codegen-mips.cc:1270: // it gets implemented.
Remove this comment, 'mls' refers to arm instruction. There is a possibility
that this could be implemented with mips multiply-and-add but you would have to
change the sign first, and that wouldn't end up saving any instructions.

[palfia, 2013-06-15 00:15:25]

Hi Jakob and Paul,

I addressed your comments and also added a fix for MIPS to
hydrogen-instructions.cc. The MathFloorOfDiv on MIPS can only use Constant as
right operand, this fix makes sure, that only Constant operand will be passed to
MathFloorOfDiv as right op.

PTAL.

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips.cc
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips...
src/mips/lithium-codegen-mips.cc:1187: 
On 2013/06/14 16:34:58, Paul Lind wrote:
> Is there any specific reason you removed this assertion here:
> ASSERT(LChunkBuilder::HasMagicNumberForDivisor(divisor));

Added ASSERT.

https://codereview.chromium.org/16951016/diff/1/src/mips/lithium-codegen-mips...
src/mips/lithium-codegen-mips.cc:1270: // it gets implemented.
On 2013/06/14 16:34:58, Paul Lind wrote:
> Remove this comment, 'mls' refers to arm instruction. There is a possibility
> that this could be implemented with mips multiply-and-add but you would have
to
> change the sign first, and that wouldn't end up saving any instructions. 

Done.

[Paul Lind, 2013-06-17 04:27:27]

Jakob, we followed the arm port too closely here, and we do not have the same
integer division limitation. Ákos is working on generalizing the routine for all
right operand values.

If you do see a trunk-push coming early Monday, perhaps you could l-g-t-m this,
since it un-breaks the build for MIPS, and we could fix it up quickly with
another CL. Otherwise we will revise this CL later in the day. 

(and we'll land it if we don't change any non-MIPS files since your previous
l-g-t-m.)

https://codereview.chromium.org/16951016/diff/13001/src/mips/lithium-codegen-...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/16951016/diff/13001/src/mips/lithium-codegen-...
src/mips/lithium-codegen-mips.cc:1340: // integer division routine is usually
slower than transitionning to FPU.
This statement is not true for mips, where we have integer division on all
cores. We will refactor this routine to work in all cases, and remove the ifdef
in hydrogen-instructions.

[Jakob Kummerow, 2013-06-17 07:07:00]

On 2013/06/17 04:27:27, Paul Lind wrote:
> Jakob, we followed the arm port too closely here, and we do not have the same
> integer division limitation. Ákos is working on generalizing the routine for
all
> right operand values.

Great; I was about to suggest just that. You can probably follow the ia32 port
in this case.

(FWIW, the ARM port should be generalized too, using VFP when SDIV is not
available, but that's obviously not your job.)

> If you do see a trunk-push coming early Monday, perhaps you could l-g-t-m
this,
> since it un-breaks the build for MIPS, and we could fix it up quickly with
> another CL. Otherwise we will revise this CL later in the day. 

Note that 3.19.17 (trunk @15175) is the same as 3.19.15.1 + r15167, so it should
build just fine on MIPS. 3.19.16.1 had stability issues so we had to revert that
for now.

[palfia, 2013-06-17 11:31:37]

Hi Jakob and Paul,

I uploaded the new, more generic MathFloorOfDiv, and removed the restriction
from hydrogen-instructions.cc.

PTAL.

https://codereview.chromium.org/16951016/diff/13001/src/mips/lithium-codegen-...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/16951016/diff/13001/src/mips/lithium-codegen-...
src/mips/lithium-codegen-mips.cc:1340: // integer division routine is usually
slower than transitionning to FPU.
On 2013/06/17 04:27:28, Paul Lind wrote:
> This statement is not true for mips, where we have integer division on all
> cores. We will refactor this routine to work in all cases, and remove the
ifdef
> in hydrogen-instructions.

Done.

[Jakob Kummerow, 2013-06-17 11:55:10]

Hydrogen and mjsunit changes still LGTM.

Glancing at src/mips/lithium-codegen-mips.cc, I'm a bit surprised that you've
opted for quite different implentations of DoDivI and DoMathFloorOfDiv. On Intel
platforms, the idea is that these two are very similar (which is why there, the
difference is just a flag). But as long as it works and fulfills your
expectations of runtime performance and code maintainability, that's fine with
me :-)

[palfia, 2013-06-17 13:05:15]

On 2013/06/17 11:55:10, Jakob wrote:
> Hydrogen and mjsunit changes still LGTM.
> 
> Glancing at src/mips/lithium-codegen-mips.cc, I'm a bit surprised that you've
> opted for quite different implentations of DoDivI and DoMathFloorOfDiv. On
Intel
> platforms, the idea is that these two are very similar (which is why there,
the
> difference is just a flag). But as long as it works and fulfills your
> expectations of runtime performance and code maintainability, that's fine with
> me :-)

We are open for any improvement idea. :-)

However, I just looked into the Intel implementation of DoDivI and
DoMathFloorOfDiv and as far as I can see, they are quite different.

On MIPS, I copied a part of the DoDivI to the DoMathFloorOfDiv's
!IsConstantOperand() branch and only changed the truncating parts in
DoMathFloorOfDiv. Of course, we could create a common static inline function for
the common parts of DoDivI and DoMathFloorOfDiv, but we are unsure if it is
worth it in this particular case.

[Jakob Kummerow, 2013-06-17 13:47:17]

On 2013/06/17 13:05:15, palfia wrote:
> However, I just looked into the Intel implementation of DoDivI and
> DoMathFloorOfDiv and as far as I can see, they are quite different.

OK, yeah, they are. I was speaking from memory. What I meant was the case of
floor-of-div with non-constant divisor, which is handled by plain DivI (+
is_flooring() flag) on ia32, but folded into DoFloorOfDiv on ARM and MIPS. I'm
not actually sure what the cleaner implementation is.

[Paul Lind, 2013-06-17 14:26:02]

LGTM, thanks for the fixes.

[palfia, 2013-06-17 15:06:51]

Committed manually as r15181 (presubmit successful).