X64 Win64: Reimplement fmod so that it works.

X64 Win64: Reimplement fmod so that it works.

[Lasse Reichstein, 2009-10-19 13:03:21]

Kevin, could you please check the frame-interaction in codegen-x64.cc /
GenerateNumberMod() ?
I have a hunch that there is a problem there.

[Kevin Millikin (Chromium), 2009-10-19 15:42:15]

We should meet to see if we can figure out a simpler way to do this.  It's
pretty complicated.

I think the virtual frame operations are OK because you happen to know the only
call site to %_NumberMod.  I'm more worried about returning the immutable heap
numbers.  We should convince ourselves that's OK (if it is).

Comments below.

http://codereview.chromium.org/300004/diff/1/11
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/300004/diff/1/11#newcode289
Line 289: InvalidOperandException = 0x01,
These should be named like kInvalidOperandException.

http://codereview.chromium.org/300004/diff/1/12
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/300004/diff/1/12#newcode4011
Line 4011: Result tmp = allocator_->Allocate(rax);
In general it's not safe to allocate registers and hold onto them while
generating code for a subexpression.  The code snippets for expressions all
assume that all live registers are held by the frame so they can be spilled.

This will fail if the subexpression tries to allocate rax (eg, a recursive call
to %_NumberMod).  In this case it happens to work because you know the only call
site to %_NumberMod, but you should not rely on it.

The usual idiom is to emit code for the subexpressions (via Load) and then
allocate any fixed registers you need.

http://codereview.chromium.org/300004/diff/1/12#newcode4023
Line 4023: frame_->SpillAll();  // Code uses the physical stack, so it must be
in sync.
This should probably be a SpilledScope to make it absolutely clear where the
spilled assumption starts and stops.

http://codereview.chromium.org/300004/diff/1/12#newcode4034
Line 4034: // Load divisor.
Doesn't FloatingPointHelper::LoadFloatOperand do exactly this?

http://codereview.chromium.org/300004/diff/1/12#newcode4048
Line 4048: // Load dividend.
Ditto FloatingPointHelper::LoadFloatOperand.

http://codereview.chromium.org/300004/diff/1/12#newcode4084
Line 4084: __ Move(dividend, Factory::nan_value());
Is it really safe to return the immutable NaN value (since we overwrite the
results of arithmetic operations)?  It seems fishy to me.

http://codereview.chromium.org/300004/diff/1/12#newcode4170
Line 4170: __ AllocateInNewSpace(HeapNumber::kSize,
I think FloatingPointHelper::AllocateHeapNumber does all this?

http://codereview.chromium.org/300004/diff/1/12#newcode4173
Line 4173: divisor,
This code should be made to handle the case where dividend and divisor are the
same register. %_NumberMod(x, x) would hit that case (again, I don't think this
code should rely on happening to know the only call site.)

http://codereview.chromium.org/300004/diff/1/12#newcode4183
Line 4183: frame_->Push(rax);
You allocated tmp, you should Push(&tmp).  (Alternately, you don't need to
allocate tmp and callee_save_result, since you can freely use registers in a
spilled scope).

[Lasse Reichstein, 2009-10-20 10:39:11]

Bill, please review the change in general.
Kevin, could you do a quick check that I have followed your directions
correctly.

[William Hesse, 2009-10-20 14:50:51]

http://codereview.chromium.org/300004/diff/5020/4003
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/300004/diff/5020/4003#newcode379
Line 379: // Potentially alternative support for modulo operation on numbers.
I don't understand this comment.  What is it, when is it used, what does it do?

http://codereview.chromium.org/300004/diff/5020/4012
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/300004/diff/5020/4012#newcode4013
Line 4013: 
Could this function be documented more, explaining what it does, how it does it,
and why it does it?

http://codereview.chromium.org/300004/diff/5020/4015
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/300004/diff/5020/4015#newcode1316
Line 1316: 
Can this load optimization, and the Operand + displacement constructor, be moved
out to a separate changelist?  That way we can see if it is worth it.

[William Hesse, 2009-10-20 14:56:38]

Isn't there a simpler way to do this?  If not, say why in the comments.

[Lasse Reichstein, 2009-10-22 11:22:53]

I think there is a simpler way (that also fixes other uses of fmod), and I'm
trying that instead.

http://codereview.chromium.org/300004/diff/5020/4003
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/300004/diff/5020/4003#newcode379
Line 379: // Potentially alternative support for modulo operation on numbers.
It generates code to compute x % y. It's "potential" because it should only be
used on 64-bit Windows.

http://codereview.chromium.org/300004/diff/5020/4015
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/300004/diff/5020/4015#newcode1316
Line 1316: 
I'm dropping this approach, going for a replacement of fmod at the C++ level
instead, but I'll save this change and see if it makes sense on its own.