Move ToI conversions to the MacroAssembler

Move ToI conversions to the MacroAssembler

+ Replace DeferredTaggedToINoSSE2 by DoubleToIStub and a fpu version.

+ Prevent truncating TaggedToI from bailing out.

BUG=
R=verwaest@chromium.org

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

[danno, 2013-08-09 13:27:24]

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:279: void MacroAssembler::TaggedToI(Register
input_reg, Register result_reg,
Why not just pass is no_reg to temp (removing the Maybe<>) argument for cases
that don't have a pre-allocated temp? I think that makes the interface much
cleaner and that's what we do elsewhere.

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:287: if (CpuFeatures::IsSafeForSnapshot(SSE2)
&& temp.has_value) {
if temp == no_reg, then why not just just do:

temp = xmm1;
subi (esp, esp, Immediate(kDoubleSize));
movdbl(MemOperand(esp, 0), temp);

... existing code ...

addi (esp, esp, Immediate(kDoubleSize));

I think it probably will still be much faster than the x87 code.

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:344: }
Why do you need an explicit done label? Falling through should be the "done"
continuation after the current instruction without an explicit jump is cleaner,
that's what seems natural for  most cases above and I think adding a jmp to pc+4
is pretty silly in those cases.

[oliv, 2013-08-12 12:18:46]

ptal

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:279: void MacroAssembler::TaggedToI(Register
input_reg, Register result_reg,
On 2013/08/09 13:27:25, danno wrote:
> Why not just pass is no_reg to temp (removing the Maybe<>) argument for cases
> that don't have a pre-allocated temp? I think that makes the interface much
> cleaner and that's what we do elsewhere.

Done.

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:287: if (CpuFeatures::IsSafeForSnapshot(SSE2)
&& temp.has_value) {
On 2013/08/09 13:27:25, danno wrote:
> if temp == no_reg, then why not just just do:
> 
> temp = xmm1;
> subi (esp, esp, Immediate(kDoubleSize));
> movdbl(MemOperand(esp, 0), temp);
> 
> ... existing code ...
> 
> addi (esp, esp, Immediate(kDoubleSize));
> 
> I think it probably will still be much faster than the x87 code.

not sure what you mean by existing code. but actually we currently always have a
temp, so lets just assert that.

https://codereview.chromium.org/22290005/diff/33001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:344: }
On 2013/08/09 13:27:25, danno wrote:
> Why do you need an explicit done label? Falling through should be the "done"
> continuation after the current instruction without an explicit jump is
cleaner,
> that's what seems natural for  most cases above and I think adding a jmp to
pc+4
> is pretty silly in those cases.

Done.

[danno, 2013-08-13 13:04:20]

https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:281: XMMRegister temp, bool
bailout_on_minus_zero, Label* bailout) {
perhaps a better name than bailout? lost_precision? and bailout_on_minus_zero ->
treat_minus_zero_as_zero?

https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.h:484: void TaggedToI(Register input_reg, Register
result_reg,
I think it's better to not have this second version at all. Callers should be
forced to think about whether they are passing a temp register or not.

[oliv, 2013-08-20 15:14:44]

On 2013/08/13 13:04:20, danno wrote:
>
https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
> File src/ia32/macro-assembler-ia32.cc (right):
> 
>
https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
> src/ia32/macro-assembler-ia32.cc:281: XMMRegister temp, bool
> bailout_on_minus_zero, Label* bailout) {
> perhaps a better name than bailout? lost_precision? and bailout_on_minus_zero
->
> treat_minus_zero_as_zero?
> 
>
https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
> File src/ia32/macro-assembler-ia32.h (right):
> 
>
https://codereview.chromium.org/22290005/diff/44001/src/ia32/macro-assembler-...
> src/ia32/macro-assembler-ia32.h:484: void TaggedToI(Register input_reg,
Register
> result_reg,
> I think it's better to not have this second version at all. Callers should be
> forced to think about whether they are passing a temp register or not.

ptal

[danno, 2013-08-20 16:14:28]

Very close, I like this CL a lot.

About testing: Can you make sure that all of the code paths are hit that you've
added? i.e. add single OS::DebugBreaks one after another in
macro-assembler-ia32.cc to the different branches you've created, and run the
unit tests to make sure that each one gets hit during the test suite run. If
not, add tests to make sure that all the code is covered.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2668: true, &call_runtime);
nit: odd indentation

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

https://codereview.chromium.org/22290005/diff/65011/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5302: (instr->temp() != NULL) ?
ToDoubleRegister(instr->temp()) : no_xmm_reg,
nit: factor this out into a variable that is passed in into TaggedToI. It makes
debugging and readability better.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:407: j(not_equal, !treat_minus_zero_as_zero
nit: Factor this label decision out into a variable that you pass to this
statement and the next.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.h:478: int index = HeapNumber::kValueOffset -
kHeapObjectTag);
nit: this parameter should be called offset, right?

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.h:485: XMMRegister scratch, bool
treat_minus_zero_as_zero,
Can you convert this bool into an enum?

enum MinusZeroMode {
  TREAT_MINUS_ZERO_AS_ZERO,
  FAIL_ON_MINUS_ZERO
}

[oliv, 2013-08-20 16:56:23]

> About testing: Can you make sure that all of the code paths > are hit that
you've added?

The situation is the following: everything has good coverage except for
X87TOSToI and TruncateX87TOSToI, which is atm dead code, therefore not testable.

It will be extensively used and covered in the coming hydrogen binopStubs. If
you don't like that, i can rip them out and keep them for the binop cl.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2668: true, &call_runtime);
On 2013/08/20 16:14:28, danno wrote:
> nit: odd indentation

Done.

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

https://codereview.chromium.org/22290005/diff/65011/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5302: (instr->temp() != NULL) ?
ToDoubleRegister(instr->temp()) : no_xmm_reg,
On 2013/08/20 16:14:28, danno wrote:
> nit: factor this out into a variable that is passed in into TaggedToI. It
makes
> debugging and readability better.

Done.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:407: j(not_equal, !treat_minus_zero_as_zero
On 2013/08/20 16:14:28, danno wrote:
> nit: Factor this label decision out into a variable that you pass to this
> statement and the next.

Done.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.h:478: int index = HeapNumber::kValueOffset -
kHeapObjectTag);
On 2013/08/20 16:14:28, danno wrote:
> nit: this parameter should be called offset, right?

Done.

https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.h:485: XMMRegister scratch, bool
treat_minus_zero_as_zero,
On 2013/08/20 16:14:28, danno wrote:
> Can you convert this bool into an enum?
> 
> enum MinusZeroMode {
>   TREAT_MINUS_ZERO_AS_ZERO,
>   FAIL_ON_MINUS_ZERO
> }

Done.

[Toon Verwaest, 2013-08-27 14:49:28]

I like this change, but there seem to still be issues with it. If missing, can
you add tests for all the cases that I mentioned that are actually broken?

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2382: __ TruncateHeapNumberToI(edx, ecx);
I'd prefer to have the target on the left, as we have everywhere else (eg __
mov(ecx, edx)).

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2383: __ mov(edx, ecx);
Why is edx overwritten by ecx here, rather than TruncateHeapNumberToI(edx, edx);

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2669: __ jmp(&int_exponent);
Unconditional jump with unlabeled code below?

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

https://codereview.chromium.org/22290005/diff/80001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5428: instr->hydrogen()->GetMinusZeroMode(),
&bailout, Label::kNear);
This DoDoubleToSmi just seems to do DoubleToI? Missing SmiTag at the end?

[oliv, 2013-08-28 08:28:54]

PTAL. comments addressed and arguments swapped to conform with the order /most/
other instructions have. we discussed fixing the order in even more
instructions, but i think this should be done separately.

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2382: __ TruncateHeapNumberToI(edx, ecx);
On 2013/08/27 14:49:28, Toon Verwaest wrote:
> I'd prefer to have the target on the left, as we have everywhere else (eg __
> mov(ecx, edx)).

Done.

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2383: __ mov(edx, ecx);
On 2013/08/27 14:49:28, Toon Verwaest wrote:
> Why is edx overwritten by ecx here, rather than TruncateHeapNumberToI(edx,
edx);

mechanical refactoring...

https://codereview.chromium.org/22290005/diff/80001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:2669: __ jmp(&int_exponent);
On 2013/08/27 14:49:28, Toon Verwaest wrote:
> Unconditional jump with unlabeled code below?

mechanical refactoring again... sry

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

https://codereview.chromium.org/22290005/diff/80001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5428: instr->hydrogen()->GetMinusZeroMode(),
&bailout, Label::kNear);
On 2013/08/27 14:49:28, Toon Verwaest wrote:
> This DoDoubleToSmi just seems to do DoubleToI? Missing SmiTag at the end?

works as intended. there is a jump over the bailout code, and afterwards a
smi_tag

[Toon Verwaest, 2013-08-30 09:28:38]

lgtm

[oliv, 2013-09-02 09:31:10]

Committed patchset #8 manually as r16464 (presubmit successful).