Implement X87 stack tracking and x87 multiplication

Implement X87 stack tracking and x87 multiplication

BUG=
R=mvstanton@chromium.org

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

[mvstanton, 2013-07-04 12:30:55]

Very nice, I just have a few comments.
Thanks,
--Michael

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:567: if (st > 0) x87_stack_[i] =
x87_stack_[x87_st2idx(0)];
Could you use { } around the assignment? I'm usually okay with the one-line if
statement but with the assignment in there it seems odd.

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:633: // Flush stack from tos down, since
PopX87() will mess with tos
Update the comment to reflect that PopX87() is gone.

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:975: Label done;
Could you explain what is happening here with a comment?

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.h (right):

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.h:121: enum X87OperandType { kX87DoubleOperand,
kX87FloatOperand, kX87IntOperand };
Maybe update the comment above, it's out of date now, talking about "either 0 or
1..."

https://codereview.chromium.org/18041003/diff/45001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.h:432: X87Register x87_stack_[8];
Can you replace the constant with something more official, like from
assembler-ia32.h?

[oliv, 2013-07-05 09:44:16]

PTAL

ps: i often upload cls for my own viewing pleasure, so you might waste your time
by reviewing before i send you a mail about it. in this case i was waiting for
another cl to land... thus patchset 2 contains some minor corrections. (but i'll
try not to add reviewers beforehand in the future)

[mvstanton, 2013-07-10 09:57:33]

lgtm.
Have a look at the one comment.
Very nice!
--Michael

https://chromiumcodereview.appspot.com/18041003/diff/64001/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/18041003/diff/64001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:988: for (int i = 0; i < x87_stack_depth_; i++)
__ fstp(0);
Put the x87 stack flushing operation in a method call like the other ops, which
are well encapsulated.

[oliv, 2013-07-10 12:28:56]

On 2013/07/10 09:57:33, mvstanton wrote:
> lgtm.
> Have a look at the one comment.
> Very nice!
> --Michael
> 
>
https://chromiumcodereview.appspot.com/18041003/diff/64001/src/ia32/lithium-c...
> File src/ia32/lithium-codegen-ia32.cc (right):
> 
>
https://chromiumcodereview.appspot.com/18041003/diff/64001/src/ia32/lithium-c...
> src/ia32/lithium-codegen-ia32.cc:988: for (int i = 0; i < x87_stack_depth_;
i++)
> __ fstp(0);
> Put the x87 stack flushing operation in a method call like the other ops,
which
> are well encapsulated.

sorry found some potential problems, PTAL

[mvstanton, 2013-07-11 10:43:25]

I think it looks good,
--Michael

[oliv, 2013-07-11 13:07:11]

Committed patchset #7 manually as r15628 (presubmit successful).