[turbofan] Added the RoundInt64ToFloat64 instruction to TurboFan.

[turbofan] Added the RoundInt64ToFloat64 instruction to TurboFan.

Committed: https://crrev.com/870e908d882d34aa73d197fe1d01b997fbc94156
Cr-Commit-Position: refs/heads/master@{#31752}

[ahaas, 2015-10-30 00:51:41]

ahaas@chromium.org changed reviewers:
+ rossberg@chromium.org, titzer@chromium.org

[ahaas, 2015-10-30 00:51:41]

ahaas@chromium.org changed required reviewers:
+ rossberg@chromium.org

[ahaas, 2015-10-30 00:51:42]

Hi,
I implemented the ChangeInt64ToFloat64 instruction for x64, arm64, and mips64.
It's not implemented for ia32, arm, mips, and x87.

[Benedikt Meurer, 2015-10-30 09:42:08]

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

[Benedikt Meurer, 2015-10-30 09:42:08]

What is the semantics of the conversion? Esp. what should happen if the value is
outside the safe integer range?

https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h
File src/compiler/opcodes.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h#newc...
src/compiler/opcodes.h:270: V(ChangeInt64ToFloat64)       \
This operator should not be named "ChangeFoo", because it doesn't preserve the
value and so it's not a change. It should either be named TruncateFoo if it
truncates values outside the valid range, or ConvertFoo if it does some other
black magic (although that's probably hard to get consistent across platforms).

https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:944: __
Cvtqsi2sd(i.OutputDoubleRegister(), i.InputRegister(0));
This is most likely not what you want, as you depend on the current rounding
mode. Intel manual says "When conversion is inexact, the value returned is
rounded according to the rounding control bits in the MXCSR register.".

https://codereview.chromium.org/1424333002/diff/1/src/types.h
File src/types.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
Please don't add these helpers here. That's just confusing to people reading the
code, because it's completely unobvious that Type::Signed64 is the same as
Type::Internal. With this shortcut, a FixedArray will also say that it's
signed64 for example.

[titzer, 2015-10-30 09:44:06]

Benedikt beat me by 1 min.

https://codereview.chromium.org/1424333002/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.h:195: const Operator* ChangeInt64ToFloat64();
I think we should call this "RoundInt64ToFloat64" since there is a possible loss
of precision.

[titzer, 2015-10-30 09:45:33]

On 2015/10/30 09:42:08, Benedikt Meurer wrote:
> What is the semantics of the conversion? Esp. what should happen if the value
is
> outside the safe integer range?
> 
> https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h
> File src/compiler/opcodes.h (right):
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h#newc...
> src/compiler/opcodes.h:270: V(ChangeInt64ToFloat64)       \
> This operator should not be named "ChangeFoo", because it doesn't preserve the
> value and so it's not a change. It should either be named TruncateFoo if it
> truncates values outside the valid range, or ConvertFoo if it does some other
> black magic (although that's probably hard to get consistent across
platforms).
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
> File src/compiler/x64/code-generator-x64.cc (right):
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
> src/compiler/x64/code-generator-x64.cc:944: __
> Cvtqsi2sd(i.OutputDoubleRegister(), i.InputRegister(0));
> This is most likely not what you want, as you depend on the current rounding
> mode. Intel manual says "When conversion is inexact, the value returned is
> rounded according to the rounding control bits in the MXCSR register.".
> 
> https://codereview.chromium.org/1424333002/diff/1/src/types.h
> File src/types.h (right):
> 
> https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
> src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
> Please don't add these helpers here. That's just confusing to people reading
the
> code, because it's completely unobvious that Type::Signed64 is the same as
> Type::Internal. With this shortcut, a FixedArray will also say that it's
> signed64 for example.

The motivation for adding the utility was just to add a marker until there is a
real type for Int64. Maybe it should be moved to the typer instead.

[Benedikt Meurer, 2015-10-30 09:47:22]

> > https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
> > src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
> > Please don't add these helpers here. That's just confusing to people reading
> the
> > code, because it's completely unobvious that Type::Signed64 is the same as
> > Type::Internal. With this shortcut, a FixedArray will also say that it's
> > signed64 for example.
> 
> The motivation for adding the utility was just to add a marker until there is
a
> real type for Int64. Maybe it should be moved to the typer instead.

Ok, but I'd prefer to have TODOs as markers if possible. Even if you move it
to the Typer, you still have a Type that can only be used to declare things not
to check stuff, which might not be obvious to everyone.

[titzer, 2015-10-30 09:48:29]

On 2015/10/30 09:47:22, Benedikt Meurer wrote:
> > > https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
> > > src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
> > > Please don't add these helpers here. That's just confusing to people
reading
> > the
> > > code, because it's completely unobvious that Type::Signed64 is the same as
> > > Type::Internal. With this shortcut, a FixedArray will also say that it's
> > > signed64 for example.
> > 
> > The motivation for adding the utility was just to add a marker until there
is
> a
> > real type for Int64. Maybe it should be moved to the typer instead.
> 
> Ok, but I'd prefer to have TODOs as markers if possible. Even if you move it
> to the Typer, you still have a Type that can only be used to declare things
not
> to check stuff, which might not be obvious to everyone.

sgtm

[akos.palfi.imgtec, 2015-10-30 15:42:18]

akos.palfi@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com

[akos.palfi.imgtec, 2015-10-30 15:42:19]

MIPS part L-G-T-M.

[titzer, 2015-10-30 19:43:49]

On 2015/10/30 09:42:08, Benedikt Meurer wrote:
> What is the semantics of the conversion? Esp. what should happen if the value
is
> outside the safe integer range?
> 
> https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h
> File src/compiler/opcodes.h (right):
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h#newc...
> src/compiler/opcodes.h:270: V(ChangeInt64ToFloat64)       \
> This operator should not be named "ChangeFoo", because it doesn't preserve the
> value and so it's not a change. It should either be named TruncateFoo if it
> truncates values outside the valid range, or ConvertFoo if it does some other
> black magic (although that's probably hard to get consistent across
platforms).
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
> File src/compiler/x64/code-generator-x64.cc (right):
> 
>
https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
> src/compiler/x64/code-generator-x64.cc:944: __
> Cvtqsi2sd(i.OutputDoubleRegister(), i.InputRegister(0));
> This is most likely not what you want, as you depend on the current rounding
> mode. Intel manual says "When conversion is inexact, the value returned is
> rounded according to the rounding control bits in the MXCSR register.".
> 

Doesn't the rounding mode also apply to all floating point operations as well?
In that case, I guess we are already depending on the default to be set
correctly.

> https://codereview.chromium.org/1424333002/diff/1/src/types.h
> File src/types.h (right):
> 
> https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
> src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
> Please don't add these helpers here. That's just confusing to people reading
the
> code, because it's completely unobvious that Type::Signed64 is the same as
> Type::Internal. With this shortcut, a FixedArray will also say that it's
> signed64 for example.

[ahaas, 2015-11-02 10:35:37]

https://codereview.chromium.org/1424333002/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.h:195: const Operator* ChangeInt64ToFloat64();
On 2015/10/30 09:44:06, titzer wrote:
> I think we should call this "RoundInt64ToFloat64" since there is a possible
loss
> of precision.

Done.

https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h
File src/compiler/opcodes.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/opcodes.h#newc...
src/compiler/opcodes.h:270: V(ChangeInt64ToFloat64)       \
On 2015/10/30 09:42:08, Benedikt Meurer wrote:
> This operator should not be named "ChangeFoo", because it doesn't preserve the
> value and so it's not a change. It should either be named TruncateFoo if it
> truncates values outside the valid range, or ConvertFoo if it does some other
> black magic (although that's probably hard to get consistent across
platforms).

As titzer suggested, I renamed the operator to "RoundInt64ToFloat64".

https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1424333002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:944: __
Cvtqsi2sd(i.OutputDoubleRegister(), i.InputRegister(0));
On 2015/10/30 09:42:08, Benedikt Meurer wrote:
> This is most likely not what you want, as you depend on the current rounding
> mode. Intel manual says "When conversion is inexact, the value returned is
> rounded according to the rounding control bits in the MXCSR register.".

As discussed, we will use this instruction because it satisfies the definition
of the int-to-float conversion in the IEEE 754 standard. The rounding mode is
the same as for all other float64 instructions in v8, which is round-to-nearest.

https://codereview.chromium.org/1424333002/diff/1/src/types.h
File src/types.h (right):

https://codereview.chromium.org/1424333002/diff/1/src/types.h#newcode365
src/types.h:365: static TypeImpl* Signed64() { return Internal(); }
On 2015/10/30 09:42:08, Benedikt Meurer wrote:
> Please don't add these helpers here. That's just confusing to people reading
the
> code, because it's completely unobvious that Type::Signed64 is the same as
> Type::Internal. With this shortcut, a FixedArray will also say that it's
> signed64 for example.

Done. I removed Type::Signed64 again and instead use Type::Internal.

[titzer, 2015-11-02 19:14:01]

lgtm modulo disassembler.

https://codereview.chromium.org/1424333002/diff/20001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1424333002/diff/20001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:1366: void vcvtqsi2sd(XMMRegister dst, XMMRegister src1,
const Operand& src2) {
Do you need to add disassembler support for this?

[ahaas, 2015-11-03 09:28:18]

ahaas@chromium.org changed required reviewers:
- rossberg@chromium.org

[ahaas, 2015-11-03 09:28:19]

https://codereview.chromium.org/1424333002/diff/20001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/1424333002/diff/20001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:1366: void vcvtqsi2sd(XMMRegister dst, XMMRegister src1,
const Operand& src2) {
On 2015/11/02 19:14:01, titzer wrote:
> Do you need to add disassembler support for this?

Disassembler support already exists for this one.

[Benedikt Meurer, 2015-11-03 09:29:08]

Description was changed from

==========
Added the ChangeInt64ToFloat64 instruction.
==========

to

==========
[turbofan] Added the RoundInt64ToFloat64 instruction to TurboFan.
==========

[rossberg, 2015-11-03 10:46:32]

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:436: Node* RoundInt64ToFloat64(Node* a) {
Nit: maybe move it down to the Truncate operators.

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:1998: return Type::Intersect(Type::Internal(),
Type::UntaggedFloat64(), zone());
This gives the return type, so should be Number, not Internal. (Can this ever
result in NaN or -0? If not, you can actually make it PlainNumber.)

[ahaas, 2015-11-03 15:03:38]

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:436: Node* RoundInt64ToFloat64(Node* a) {
On 2015/11/03 10:46:32, rossberg wrote:
> Nit: maybe move it down to the Truncate operators.

Done.

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/1424333002/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:1998: return Type::Intersect(Type::Internal(),
Type::UntaggedFloat64(), zone());
On 2015/11/03 10:46:32, rossberg wrote:
> This gives the return type, so should be Number, not Internal. (Can this ever
> result in NaN or -0? If not, you can actually make it PlainNumber.)

Done.

[rossberg, 2015-11-03 15:13:24]

https://codereview.chromium.org/1424333002/diff/40001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/1424333002/diff/40001/src/compiler/typer.cc#n...
src/compiler/typer.cc:2021: return Type::PlainNumber();
Ah, no, you should keep the Intersect as before, just replace the Internal part
in your earlier version. Sorry for not being clearer.

[ahaas, 2015-11-03 15:26:38]

https://codereview.chromium.org/1424333002/diff/40001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/1424333002/diff/40001/src/compiler/typer.cc#n...
src/compiler/typer.cc:2021: return Type::PlainNumber();
On 2015/11/03 15:13:24, rossberg wrote:
> Ah, no, you should keep the Intersect as before, just replace the Internal
part
> in your earlier version. Sorry for not being clearer.

Done.

[rossberg, 2015-11-03 15:28:40]

lgtm

[ahaas, 2015-11-03 15:44:13]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2015-11-03 15:44:13]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1424333002/#ps60001
(title: "Fixed the type of RoundInt64ToFloat64")

[commit-bot: I haz the power, 2015-11-03 15:44:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424333002/60001

[commit-bot: I haz the power, 2015-11-03 15:46:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-03 15:46:43]

Patchset 4 (id:??) landed as
https://crrev.com/870e908d882d34aa73d197fe1d01b997fbc94156
Cr-Commit-Position: refs/heads/master@{#31752}